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

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

Merged
merged 7 commits into from
Nov 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 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
96ea31a
more cleanup
MikeAlhayek Nov 26, 2024
37e7562
Update test/OrchardCore.Tests/OrchardCore.Users/AccountControllerTest…
MikeAlhayek Nov 26, 2024
0e5e127
Update test/OrchardCore.Tests/OrchardCore.Users/AccountControllerTest…
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)
Piedone marked this conversation as resolved.
Show resolved Hide resolved
{
_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.

Copy link
Member

Choose a reason for hiding this comment

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

Why would the username or ID be private info in this context? These logs will be inspected by developers of the platform, who already have access to the source code, and most possibly in some form to the DB as well.

Alone, this log message only communicates that something went wrong with account creation for somebody. This isn't enough information to troubleshoot the issue, unless login is completely broken.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sebastienros onces said event user is is private info and should not be logged. Here maybe it not an issue to log username since the user does not actually exists in the database.

Copy link
Member

Choose a reason for hiding this comment

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

It's private when talking about disclosure to the public. But when logging, why wouldn't we log to make the log entry more useful? The whole log is private information, to begin with.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah but if a user asks for their data to be removed, its not simply to remove their info from the logs which puts you at the risk of not being complaint.

Copy link
Member

Choose a reason for hiding this comment

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

I would say it's fine to mask a value. For example with a username, keep the first/last char and output something like s******s (with a fixed number of stars)` and an email keeping the domain part. Note that this could be a service later as this needs more design than just through a generic masking service. There are utilities in the dotnet extensions repo for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Piedone we can "in another PR" start logging masked private data. That PR will provide the masking service or static extensions for making different things like Id, Username, email... but I need to finish this PR to address immediate problem.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't want to do a masking service here. Good point about the necessity to remove user accounts. However, user IDs don't fall under this and thus can be logged.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's move this good convo into a dedicated issue please: #17075


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)
Piedone marked this conversation as resolved.
Show resolved Hide resolved
{
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