Increase Logging in External Login and return correct result when failing to create user #17068
Conversation
Piedone
changed the title
| @@ -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) | |||
There was a problem hiding this comment.
When is it possible that we reach this while !ModelState.IsValid?
There was a problem hiding this comment.
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."); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We can't log private info like username or id. The logs are helpful to understand what is happening during external login.
| @@ -520,6 +520,75 @@ public async Task<IActionResult> RemoveLogin(RemoveLoginViewModel model) | |||
| return RedirectToAction(nameof(ExternalLogins)); | |||
| } | |||
|
|
|||
| private async Task UpdateAndValidatePasswordAsync(RegisterExternalLoginViewModel model, bool noPassword) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 (!identityResult.Succeeded) | ||
| { | ||
| if (hasPassword) | ||
| { | ||
| _logger.LogInformation(3, "Unable to create a new account with password."); |
There was a problem hiding this comment.
If you use the event ID parameter then it should be unique. I don't think it's particularly useful, though.
There was a problem hiding this comment.
Agreed that it not useful. It used like this is many places so I left it as is.
| if (!identityResult.Succeeded) | ||
| { | ||
| if (hasPassword) | ||
| { | ||
| _logger.LogInformation(3, "Unable to create a new account with password."); |
There was a problem hiding this comment.
I think these log entries would be more useful if the username was included, and for the fail ones also the errors.
There was a problem hiding this comment.
As mentioned above, we can't log private info such as username.
No description provided.