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.

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

4.11.0-beta1 PHP 7.4 - No confirmation email recieved after resetting password #10306

Closed
1 task done
GuySartorelli opened this issue May 9, 2022 · 3 comments · Fixed by #10313
Closed
1 task done

Comments

@GuySartorelli
Copy link
Member

GuySartorelli commented May 9, 2022

I used the "I've lost my password" link to have an email sent to me, which contained a link to reset my password.
I reset my password successfully.
I did not received an email confirming my password was changed.

Regression testing step:

I should receive an email confirming my password was changed

Notes

  • The first email was received, so email on SC AWS 7.4 does work

ACs

  • I get a second email saying that my password was changed

PRs

@GuySartorelli
Copy link
Member Author

This functionality is present already. It requires the site to be live, and the SilverStripe\Security\Member.notify_password_change config to be set to true. It is false by default.

// We don't send emails out on dev/tests sites to prevent accidentally spamming users.
// However, if TestMailer is in use this isn't a risk.
// @todo some developers use external tools, so emailing might be a good idea anyway
if ((Director::isLive() || Injector::inst()->get(Mailer::class) instanceof TestMailer)
&& $this->isChanged('Password')
&& $this->record['Password']
&& $this->Email
&& static::config()->get('notify_password_change')
&& $this->isInDB()
) {
Email::create()
->setHTMLTemplate('SilverStripe\\Control\\Email\\ChangePasswordEmail')
->setData($this)
->setTo($this->Email)
->setSubject(_t(
__CLASS__ . '.SUBJECTPASSWORDCHANGED',
"Your password has been changed",
'Email subject'
))
->send();
}

This doesn't seem to be documented anywhere. I can't think of a reason why it would be false - it seems to me that it should be true by default for enhanced security, and developers can turn it off if they have some aversion to it.

In any case we should update our regression test instructions to set this to true before deploying to the test environment.

@GuySartorelli
Copy link
Member Author

After a quick discussion on Slack it's been decided that changing the config value to true (and mentioning it in the changelog) is the way to go.

@emteknetnz
Copy link
Member

Linked PR has been merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants