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

Increase Logging in External Login and return correct result when failing to create user #17068

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+133 −80
Open

Increase Logging in External Login and return correct result when failing to create user #17068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
05ff0e1
Increate Logging in External Login and return correct result when fai…
MikeAlhayek Nov 25, 2024
a31ef89
cleanup
MikeAlhayek Nov 26, 2024
5927034
cleanup
MikeAlhayek Nov 26, 2024
6c85e2c
better logs
MikeAlhayek Nov 26, 2024
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
117 changes: 93 additions & 24 deletions src/OrchardCore.Modules/OrchardCore.Users/Controllers/ExternalAuthenticationsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -32,6 +33,7 @@ public sealed class ExternalAuthenticationsController : AccountBaseController
private readonly ISiteService _siteService;
private readonly IEnumerable<ILoginFormEvent> _accountEvents;
private readonly IShellFeaturesManager _shellFeaturesManager;
private readonly IEmailAddressValidator _emailAddressValidator;
private readonly IUserService _userService;
private readonly INotifier _notifier;
private readonly IEnumerable<IExternalLoginEventHandler> _externalLoginHandlers;
Expand All @@ -52,6 +54,7 @@ public ExternalAuthenticationsController(
IStringLocalizer<ExternalAuthenticationsController> stringLocalizer,
IEnumerable<ILoginFormEvent> accountEvents,
IShellFeaturesManager shellFeaturesManager,
IEmailAddressValidator emailAddressValidator,
IUserService userService,
INotifier notifier,
IEnumerable<IExternalLoginEventHandler> externalLoginHandlers,
Expand All @@ -66,6 +69,7 @@ public ExternalAuthenticationsController(
_siteService = siteService;
_accountEvents = accountEvents;
_shellFeaturesManager = shellFeaturesManager;
_emailAddressValidator = emailAddressValidator;
_userService = userService;
_notifier = notifier;
_externalLoginHandlers = externalLoginHandlers;
Expand Down Expand Up @@ -215,14 +219,16 @@ public async Task<IActionResult> 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)
Copy link
Member

Choose a reason for hiding this comment

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

When is it possible that we reach this while !ModelState.IsValid?

Copy link
Member Author

Choose a reason for hiding this comment

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

The Register method can add error to the ModelState and make it invalid. Based on the implementation we return null if there was any error. But the consumer code should not depend on the implementation return value. So it better to add this check just in case the underlining implementation changes.

{
_logger.LogError("Unable to create internal account and link it to the external user.");
Copy link
Member

Choose a reason for hiding this comment

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

I think all of these log messages would be more useful if they were to include the username as well. Otherwise, unless you're testing this right there, you won't know the context.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't log private info like username or id. The logs are helpful to understand what is happening during external login.


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)
{
Expand Down Expand Up @@ -258,6 +264,8 @@ public async Task<IActionResult> ExternalLoginCallback(string returnUrl = null,
return RedirectToLogin(returnUrl);
}

_logger.LogError("Unable to add external provider to a user: {LoginProvider} provider.", info.LoginProvider);

AddErrorsToModelState(identityResult.Errors);
}

Expand Down Expand Up @@ -294,25 +302,11 @@ public async Task<IActionResult> 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()
Expand All @@ -322,9 +316,9 @@ public async Task<IActionResult> 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));
var identityResult = await _userManager.AddLoginAsync(iUser, new UserLoginInfo(info.LoginProvider, info.ProviderKey, info.ProviderDisplayName));

if (identityResult.Succeeded)
{
Expand Down Expand Up @@ -394,7 +388,7 @@ public async Task<IActionResult> LinkExternalLogin(LinkExternalLoginViewModel mo
}
else
{
var identityResult = await _signInManager.UserManager.AddLoginAsync(user, new UserLoginInfo(info.LoginProvider, info.ProviderKey, info.ProviderDisplayName));
var identityResult = await _userManager.AddLoginAsync(user, new UserLoginInfo(info.LoginProvider, info.ProviderKey, info.ProviderDisplayName));

if (identityResult.Succeeded)
{
Expand Down Expand Up @@ -455,6 +449,7 @@ public async Task<IActionResult> LinkLogin(string provider)
public async Task<IActionResult> LinkLoginCallback()
{
var user = await _userManager.GetUserAsync(User);

if (user == null)
{
_logger.LogError("Unable to load user with ID '{UserId}'.", _userManager.GetUserId(User));
Expand All @@ -463,6 +458,7 @@ public async Task<IActionResult> LinkLoginCallback()
}

var info = await _signInManager.GetExternalLoginInfoAsync();

if (info == null)
{
_logger.LogError("Unexpected error occurred loading external login info for user '{UserName}'.", user.UserName);
Expand All @@ -471,6 +467,7 @@ public async Task<IActionResult> 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);
Expand All @@ -480,6 +477,7 @@ public async Task<IActionResult> 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);

Expand All @@ -491,6 +489,7 @@ public async Task<IActionResult> LinkLoginCallback()
public async Task<IActionResult> 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));
Expand All @@ -499,6 +498,7 @@ public async Task<IActionResult> 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);
Expand All @@ -520,6 +520,75 @@ public async Task<IActionResult> RemoveLogin(RemoveLoginViewModel model)
return RedirectToAction(nameof(ExternalLogins));
}

private async Task UpdateAndValidatePasswordAsync(RegisterExternalLoginViewModel model, bool noPassword)
Copy link
Member

Choose a reason for hiding this comment

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

Why are these better here instead of the view model? Note that moving the T-strings here will need the translations to be updated too.

Copy link
Member Author

Choose a reason for hiding this comment

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

To start, I am here using the IdentityOptions to validation the password asynchronous. Using IValidateObject you can't call asynchronous code without locking. Second, the implementation depended on a value set by setting which is wired. Validating in the controller allows for asynchronous calls and also make it clear what we are validating.

{
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)
{
continue;
}

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<Microsoft.AspNetCore.Identity.SignInResult> ExternalSignInAsync(IUser user, ExternalLoginInfo info)
{
_logger.LogInformation("Attempting to do an external sign in.");
Expand Down
2 changes: 1 addition & 1 deletion src/OrchardCore.Modules/OrchardCore.Users/Handlers/UserModerationRegistrationFormEvents.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
47 changes: 5 additions & 42 deletions src/OrchardCore.Modules/OrchardCore.Users/ViewModels/RegisterExternalLoginViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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; }
Expand All @@ -22,42 +23,4 @@ public class RegisterExternalLoginViewModel : IValidatableObject

[DataType(DataType.Password)]
public string ConfirmPassword { get; set; }

public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
{
var emailAddressValidator = validationContext.GetService<IEmailAddressValidator>();
var S = validationContext.GetService<IStringLocalizer<RegisterExternalLoginViewModel>>();

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"]);
}
}
}
4 changes: 2 additions & 2 deletions src/OrchardCore/OrchardCore.Users.Abstractions/Events/UserRegisteringContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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; }

Expand Down
Loading