-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Add new email/confirmation flows #8577
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.
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
Conversation
|
For this new requireConfirmedAccount setting, are we planning on setting it explicitly in the templates or just turn it on in AddDefaultIdentity? |
|
Tests added/updated and are green again, removing WIP label |
javiercn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good.
The only real issue I have is with the way we do the confirmation link for development.
src/Identity/UI/src/Areas/Identity/Pages/V3/Account/ConfirmEmailChange.cshtml.cs
Show resolved
Hide resolved
src/Identity/UI/src/Areas/Identity/Pages/V3/Account/ConfirmEmailChange.cshtml.cs
Outdated
Show resolved
Hide resolved
src/Identity/UI/src/Areas/Identity/Pages/V3/Account/ConfirmEmailChange.cshtml.cs
Outdated
Show resolved
Hide resolved
src/Identity/UI/src/Areas/Identity/Pages/V3/Account/RegisterConfirmation.cshtml.cs
Outdated
Show resolved
Hide resolved
src/Identity/UI/src/Areas/Identity/Pages/V3/Account/RegisterConfirmation.cshtml.cs
Outdated
Show resolved
Hide resolved
src/Identity/UI/src/Areas/Identity/Pages/V4/Account/ConfirmEmailChange.cshtml.cs
Outdated
Show resolved
Hide resolved
src/Identity/UI/src/Areas/Identity/Pages/V4/Account/Manage/Email.cshtml.cs
Show resolved
Hide resolved
src/Identity/test/Identity.FunctionalTests/Pages/Account/Manage/Email.cs
Outdated
Show resolved
Hide resolved
javiercn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
|
There may be a logical error in the confirmation of email. 'OnPostChangeEmailAsync' inside 'Email.cshtml.cs' does not check to see if the email is used by another user prior to sending confirmation. something like this? public async Task<IActionResult> OnPostChangeEmailAsync()
{
///removed for brevity
if (Input.NewEmail != email)
{
var emailExists = await _userManager.FindByEmailAsync(Input.NewEmail);
if(emailExists == null)//not notifying if email sent or not.
{
var userId = await _userManager.GetUserIdAsync(user);
var code = await _userManager.GenerateChangeEmailTokenAsync(user, Input.NewEmail);
var callbackUrl = Url.Page(
"/Account/ConfirmEmailChange",
pageHandler: null,
values: new { userId = userId, email = Input.NewEmail, code = code },
protocol: Request.Scheme);
await _emailSender.SendEmailAsync(
Input.NewEmail,
"Confirm your email",
$"Please confirm your account by <a href=\"{HtmlEncoder.Default.Encode(callbackUrl)}\">clicking here</a>.");
}
StatusMessage = "Confirmation link to change email sent. Please check your email";
return RedirectToPage();
}
///removed for brevity
} |
Part of #8356
Still working on fixing/adding tests, for the default confirmation page, we generate a new confirmation link and display it only when in development.
@ajcvickers @blowdart