From 05ff0e10ffbbd127a540e594a52ff16033accb28 Mon Sep 17 00:00:00 2001 From: Mike Alhayek Date: Mon, 25 Nov 2024 15:41:58 -0800 Subject: [PATCH] Increate Logging in External Login and return correct result when fail to create user --- .../ExternalAuthenticationsController.cs | 111 ++++++++++++++---- .../UserModerationRegistrationFormEvents.cs | 2 +- .../RegisterExternalLoginViewModel.cs | 47 +------- .../Events/UserRegisteringContext.cs | 4 +- .../Services/UserService.cs | 39 ++++-- 5 files changed, 127 insertions(+), 76 deletions(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Controllers/ExternalAuthenticationsController.cs b/src/OrchardCore.Modules/OrchardCore.Users/Controllers/ExternalAuthenticationsController.cs index fe84ec18967..77516cc903f 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/Controllers/ExternalAuthenticationsController.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/Controllers/ExternalAuthenticationsController.cs @@ -9,6 +9,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using OrchardCore.DisplayManagement.Notify; +using OrchardCore.Email; using OrchardCore.Environment.Shell; using OrchardCore.Modules; using OrchardCore.Mvc.Core.Utilities; @@ -32,6 +33,7 @@ public sealed class ExternalAuthenticationsController : AccountBaseController private readonly ISiteService _siteService; private readonly IEnumerable _accountEvents; private readonly IShellFeaturesManager _shellFeaturesManager; + private readonly IEmailAddressValidator _emailAddressValidator; private readonly IUserService _userService; private readonly INotifier _notifier; private readonly IEnumerable _externalLoginHandlers; @@ -52,6 +54,7 @@ public ExternalAuthenticationsController( IStringLocalizer stringLocalizer, IEnumerable accountEvents, IShellFeaturesManager shellFeaturesManager, + IEmailAddressValidator emailAddressValidator, IUserService userService, INotifier notifier, IEnumerable externalLoginHandlers, @@ -66,6 +69,7 @@ public ExternalAuthenticationsController( _siteService = siteService; _accountEvents = accountEvents; _shellFeaturesManager = shellFeaturesManager; + _emailAddressValidator = emailAddressValidator; _userService = userService; _notifier = notifier; _externalLoginHandlers = externalLoginHandlers; @@ -215,14 +219,16 @@ public async Task ExternalLoginCallback(string returnUrl = null, }, ModelState.AddModelError); // If the registration was successful we can link the external provider and redirect the user. - if (iUser == null) + if (iUser == null || !ModelState.IsValid) { + _logger.LogError("Unable to create internal account and link it to the external user."); + await _notifier.ErrorAsync(H["Unable to create internal account and link it to the external user."]); - return RedirectToLogin(returnUrl); + return View(nameof(RegisterExternalLogin), externalLoginViewModel); } - var identityResult = await _signInManager.UserManager.AddLoginAsync(iUser, new UserLoginInfo(info.LoginProvider, info.ProviderKey, info.ProviderDisplayName)); + var identityResult = await _userManager.AddLoginAsync(iUser, new UserLoginInfo(info.LoginProvider, info.ProviderKey, info.ProviderDisplayName)); if (identityResult.Succeeded) { @@ -258,6 +264,8 @@ public async Task ExternalLoginCallback(string returnUrl = null, return RedirectToLogin(returnUrl); } + _logger.LogError("Unable to add external provider to a user: {LoginProvider} provider.", info.LoginProvider); + AddErrorsToModelState(identityResult.Errors); } @@ -294,25 +302,11 @@ public async Task RegisterExternalLogin(RegisterExternalLoginView model.NoEmail = settings.NoEmail; model.NoUsername = settings.NoUsername; - ModelState.Clear(); + UpdateAndValidateEmail(model, info, settings.NoEmail); + await UpdateAndValidateUserNameAsync(model, info, settings.NoUsername); + await UpdateAndValidatePasswordAsync(model, settings.NoPassword); - if (settings.NoEmail && string.IsNullOrWhiteSpace(model.Email)) - { - model.Email = info.GetEmail(); - } - - if (settings.NoUsername && string.IsNullOrWhiteSpace(model.UserName)) - { - model.UserName = await GenerateUsernameAsync(info); - } - - if (settings.NoPassword) - { - model.Password = null; - model.ConfirmPassword = null; - } - - if (TryValidateModel(model) && ModelState.IsValid) + if (ModelState.IsValid) { var iUser = await _userService.RegisterAsync( new RegisterUserForm() @@ -322,7 +316,7 @@ public async Task RegisterExternalLogin(RegisterExternalLoginView Password = model.Password, }, ModelState.AddModelError); - if (iUser is not null) + if (iUser is not null && ModelState.IsValid) { var identityResult = await _signInManager.UserManager.AddLoginAsync(iUser, new UserLoginInfo(info.LoginProvider, info.ProviderKey, info.ProviderDisplayName)); @@ -455,6 +449,7 @@ public async Task LinkLogin(string provider) public async Task LinkLoginCallback() { var user = await _userManager.GetUserAsync(User); + if (user == null) { _logger.LogError("Unable to load user with ID '{UserId}'.", _userManager.GetUserId(User)); @@ -463,6 +458,7 @@ public async Task LinkLoginCallback() } var info = await _signInManager.GetExternalLoginInfoAsync(); + if (info == null) { _logger.LogError("Unexpected error occurred loading external login info for user '{UserName}'.", user.UserName); @@ -471,6 +467,7 @@ public async Task LinkLoginCallback() } var result = await _userManager.AddLoginAsync(user, new UserLoginInfo(info.LoginProvider, info.ProviderKey, info.ProviderDisplayName)); + if (!result.Succeeded) { _logger.LogError("Unexpected error occurred adding external login info for user '{UserName}'.", user.UserName); @@ -480,6 +477,7 @@ public async Task LinkLoginCallback() // Clear the existing external cookie to ensure a clean login process. await HttpContext.SignOutAsync(IdentityConstants.ExternalScheme); + // Perform External Login SignIn. await ExternalSignInAsync(user, info); @@ -491,6 +489,7 @@ public async Task LinkLoginCallback() public async Task RemoveLogin(RemoveLoginViewModel model) { var user = await _userManager.GetUserAsync(User); + if (user == null || user is not User u) { _logger.LogError("Unable to load user with ID '{UserId}'.", _userManager.GetUserId(User)); @@ -499,6 +498,7 @@ public async Task RemoveLogin(RemoveLoginViewModel model) } var result = await _userManager.RemoveLoginAsync(user, model.LoginProvider, model.ProviderKey); + if (!result.Succeeded) { _logger.LogError("Unexpected error occurred removing external login info for user '{UserName}'.", user.UserName); @@ -520,6 +520,73 @@ public async Task RemoveLogin(RemoveLoginViewModel model) return RedirectToAction(nameof(ExternalLogins)); } + private async Task UpdateAndValidatePasswordAsync(RegisterExternalLoginViewModel model, bool noPassword) + { + if (noPassword) + { + model.Password = null; + model.ConfirmPassword = null; + + return; + } + + if (string.IsNullOrWhiteSpace(model.Password)) + { + ModelState.AddModelError(nameof(model.Password), S["Password is required!"]); + } + else if (model.Password != model.ConfirmPassword) + { + ModelState.AddModelError(nameof(model.ConfirmPassword), S["Confirm Password do not match"]); + } + else if (_userManager.PasswordValidators != null) + { + var user = new User(); + + foreach (var passwordValidator in _userManager.PasswordValidators) + { + var validationResult = await passwordValidator.ValidateAsync(_userManager, user, model.Password); + + if (!validationResult.Succeeded) + { + foreach (var error in validationResult.Errors) + { + ModelState.AddModelError(nameof(model.Password), error.Description); + } + } + } + } + } + + private async Task UpdateAndValidateUserNameAsync(RegisterExternalLoginViewModel model, ExternalLoginInfo info, bool noUsername) + { + if (noUsername && string.IsNullOrWhiteSpace(model.UserName)) + { + model.UserName = await GenerateUsernameAsync(info); + } + + if (string.IsNullOrWhiteSpace(model.UserName)) + { + ModelState.AddModelError(nameof(model.UserName), S["Username is required!"]); + } + } + + private void UpdateAndValidateEmail(RegisterExternalLoginViewModel model, ExternalLoginInfo info, bool noEmail) + { + if (noEmail && string.IsNullOrWhiteSpace(model.Email)) + { + model.Email = info.GetEmail(); + } + + if (string.IsNullOrWhiteSpace(model.Email)) + { + ModelState.AddModelError(nameof(model.Email), S["Email is required!"]); + } + else if (!_emailAddressValidator.Validate(model.Email)) + { + ModelState.AddModelError(nameof(model.Email), S["Invalid Email."]); + } + } + private async Task ExternalSignInAsync(IUser user, ExternalLoginInfo info) { _logger.LogInformation("Attempting to do an external sign in."); diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Handlers/UserModerationRegistrationFormEvents.cs b/src/OrchardCore.Modules/OrchardCore.Users/Handlers/UserModerationRegistrationFormEvents.cs index 00315f60a9a..2db03525555 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/Handlers/UserModerationRegistrationFormEvents.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/Handlers/UserModerationRegistrationFormEvents.cs @@ -18,7 +18,7 @@ public override Task RegisteringAsync(UserRegisteringContext context) if (context.User is User user && !(_registrationOptions.UsersAreModerated && !user.IsEnabled)) { - context.Cancel = true; + context.CancelSignIn = true; } return Task.CompletedTask; diff --git a/src/OrchardCore.Modules/OrchardCore.Users/ViewModels/RegisterExternalLoginViewModel.cs b/src/OrchardCore.Modules/OrchardCore.Users/ViewModels/RegisterExternalLoginViewModel.cs index f72c974163e..39760abb529 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/ViewModels/RegisterExternalLoginViewModel.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/ViewModels/RegisterExternalLoginViewModel.cs @@ -1,16 +1,17 @@ using System.ComponentModel.DataAnnotations; -using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Localization; -using OrchardCore.Email; +using Microsoft.AspNetCore.Mvc.ModelBinding; namespace OrchardCore.Users.ViewModels; -public class RegisterExternalLoginViewModel : IValidatableObject +public class RegisterExternalLoginViewModel { + [BindNever] public bool NoUsername { get; set; } + [BindNever] public bool NoEmail { get; set; } + [BindNever] public bool NoPassword { get; set; } public string UserName { get; set; } @@ -22,42 +23,4 @@ public class RegisterExternalLoginViewModel : IValidatableObject [DataType(DataType.Password)] public string ConfirmPassword { get; set; } - - public IEnumerable Validate(ValidationContext validationContext) - { - var emailAddressValidator = validationContext.GetService(); - var S = validationContext.GetService>(); - - if (string.IsNullOrWhiteSpace(Email)) - { - if (!NoEmail) - { - yield return new ValidationResult(S["Email is required!"], ["Email"]); - } - } - else if (!emailAddressValidator.Validate(Email)) - { - yield return new ValidationResult(S["Invalid Email."], ["Email"]); - } - - if (string.IsNullOrWhiteSpace(UserName) && !NoUsername) - { - yield return new ValidationResult(S["Username is required!"], ["UserName"]); - } - - if (string.IsNullOrWhiteSpace(Password) && !NoPassword) - { - yield return new ValidationResult(S["Password is required!"], ["Password"]); - } - - if (Password != ConfirmPassword) - { - yield return new ValidationResult(S["Confirm Password do not match"], ["ConfirmPassword"]); - } - - if (Password != null && (Password.Length < 6 || Password.Length > 100)) - { - yield return new ValidationResult(string.Format(S["Password must be between {0} and {1} characters"], 6, 100), ["Password"]); - } - } } diff --git a/src/OrchardCore/OrchardCore.Users.Abstractions/Events/UserRegisteringContext.cs b/src/OrchardCore/OrchardCore.Users.Abstractions/Events/UserRegisteringContext.cs index 806f533a372..e6e05ffb5de 100644 --- a/src/OrchardCore/OrchardCore.Users.Abstractions/Events/UserRegisteringContext.cs +++ b/src/OrchardCore/OrchardCore.Users.Abstractions/Events/UserRegisteringContext.cs @@ -1,8 +1,8 @@ -namespace OrchardCore.Users.Events; +namespace OrchardCore.Users.Events; public class UserRegisteringContext { - public bool Cancel { get; set; } + public bool CancelSignIn { get; set; } public IUser User { get; } diff --git a/src/OrchardCore/OrchardCore.Users.Core/Services/UserService.cs b/src/OrchardCore/OrchardCore.Users.Core/Services/UserService.cs index 01a444f2dcd..000ea6edfa6 100644 --- a/src/OrchardCore/OrchardCore.Users.Core/Services/UserService.cs +++ b/src/OrchardCore/OrchardCore.Users.Core/Services/UserService.cs @@ -117,16 +117,38 @@ public async Task CreateUserAsync(IUser user, string password, Action GetForgotPasswordUserAsync(string userId) { if (string.IsNullOrWhiteSpace(userId)) { - return await Task.FromResult(null); + return null; } var user = await GetUserAsync(userId); if (user == null) { - return await Task.FromResult(null); + return null; } if (user is User u) @@ -328,23 +350,22 @@ public async Task RegisterAsync(RegisterUserForm model, Action e.RegisteringAsync(ctx), context, _logger); - if (!context.Cancel) + if (!context.CancelSignIn) { await _signInManager.SignInAsync(user, isPersistent: false); } - _logger.LogInformation(3, "User created a new account with password."); - await _registrationFormEvents.InvokeAsync((e, user) => e.RegisteredAsync(user), user, _logger); return user;