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
Reset Password #1662
Reset Password #1662
Conversation
Fixes #1299
It breaks the AccountControllerTests. |
Also, I don't know how to make a Missing Banner when the email module is enabled but not configured. |
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.
It needs a little more security checks.
[AllowAnonymous] | ||
public async Task<IActionResult> ForgotPassword(ForgotPasswordViewModel model) | ||
{ | ||
if (ModelState.IsValid) |
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.
You can send email without checking
_siteService.GetSiteSettingsAsync()).As<RegistrationSettings>().EnableLostPassword
via for instance Postman.
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.
TODO
[ValidateAntiForgeryToken] | ||
public async Task<IActionResult> ResetPassword(ResetPasswordViewModel model) | ||
{ | ||
if (ModelState.IsValid) |
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.
And then you can reset password without checking
_siteService.GetSiteSettingsAsync()).As<RegistrationSettings>().EnableLostPassword
via for instance Postman.
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.
but hmm... AntiForgeryToken can protect it... but you can use tokens and cookies from another form. :)
isn't this like #1332? |
Yes, @sipronunciaaigor . I started from the code in your PR because i didn't see activity to add the missing email feature. I commented on #1299 to add important requirements. |
Yes, sorry @agriffard. As I said in that PR, I didn't watch the thread that much any more because I waited the email module for a couple of months or so on... I opened the project again just during Easter... |
Ok, we will. Thank you for your work @sipronunciaaigor . |
ISiteService siteService, | ||
ISmtpService smtpService, | ||
ICompositeViewEngine viewEngine, | ||
IStringLocalizer<AdminController> stringLocalizer, |
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.
IStringLocalizer<AccountController>
and also on html localizer
message.To.Add(user.Email); | ||
message.Subject = T["Reset your password"]; | ||
|
||
ISite site = await _siteService.GetSiteSettingsAsync(); |
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.
var
|
||
ISite site = await _siteService.GetSiteSettingsAsync(); | ||
|
||
string siteUrl = site.BaseUrl; |
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.
Because the email is sent from a controller you don't need BaseUrl
.
Create the link using Url.Action
and the route values (not the string "/OrchardCore.Users/Account/ResetPassword"
) then pass the protocol: Request.Scheme
argument which will tell the helper to generate a fully qualified url using the current protocol.
@@ -6,6 +6,7 @@ | |||
</PropertyGroup> | |||
|
|||
<ItemGroup> | |||
<ProjectReference Include="..\..\OrchardCore.Modules\OrchardCore.Email\OrchardCore.Email.csproj" /> |
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.
Remove that, only a reference to the abstractions
user = await _userManager.FindByEmailAsync(userIdentifier); | ||
} | ||
|
||
return await Task.FromResult(user); |
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.
return user
OK, I made changes according to the review. It works on https://localhost:44300/ |
User = user, | ||
LostPasswordUrl = Url.Action("ResetPassword", "Account", new { code = user.ResetToken }, HttpContext.Request.Scheme) | ||
}; | ||
// Todo: Find a way to render a shape |
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.
Remove comment
var viewName = "TemplateUserLostPassword"; | ||
var model = Arguments.From(new { User = user, LostPasswordUrl = Url.Action("ResetPassword", "Account", new { code = user.ResetToken }, HttpContext.Request.Scheme) }); | ||
|
||
var options = ControllerContext.HttpContext.RequestServices.GetRequiredService<IOptions<MvcViewOptions>>(); |
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.
I doubt this is the simplest way to get the html ... I will look into it.
Fixes #1299
I still don't know how to separate the Users module from the Email one.
I would like to use ShapeFactory and DisplayFactory to generate a string from a template but for the moment, I generate it thanks to the ViewEngine.
It would be interesting to have helper or extension methods to generate urls using BaseUrl SiteSettings and make absolute url.