From ca4036b882e37607debc52b2c76f3d6641e34078 Mon Sep 17 00:00:00 2001 From: Roman Schmid Date: Mon, 4 Apr 2016 11:48:56 +0200 Subject: [PATCH] Removed Email Subclasses used by the Member class (Member_ChangePasswordEmail and Member_ForgotPasswordEmail). Added a test for the forgot password email. Improved the test for the change-password email. Fixed issue where `SapphireTest::mailer` was cleared during `setUp` by moving instantiation of the mailer at the end of the `setUp` method. No longer use deprecated i18n method in test-setup. Replace potentially real Email Address with a fake one. --- dev/SapphireTest.php | 14 ++++---- security/Member.php | 45 +++---------------------- security/MemberLoginForm.php | 5 ++- templates/email/ChangePasswordEmail.ss | 2 +- tests/security/MemberTest.php | 36 +++++++++++++++++--- tests/security/MemberTest.yml | 2 +- tests/security/SecurityTest.php | 46 +++++++++++++------------- 7 files changed, 72 insertions(+), 78 deletions(-) diff --git a/dev/SapphireTest.php b/dev/SapphireTest.php index de475f8b5..4fb2310c4 100644 --- a/dev/SapphireTest.php +++ b/dev/SapphireTest.php @@ -192,7 +192,7 @@ class SapphireTest extends PHPUnit_Framework_TestCase { self::$is_running_test = true; // i18n needs to be set to the defaults or tests fail - i18n::set_locale(i18n::default_locale()); + i18n::set_locale(Config::inst()->get('i18n', 'default_locale')); i18n::config()->date_format = null; i18n::config()->time_format = null; @@ -223,12 +223,6 @@ class SapphireTest extends PHPUnit_Framework_TestCase { $prefix = defined('SS_DATABASE_PREFIX') ? SS_DATABASE_PREFIX : 'ss_'; - // Set up email - $this->originalMailer = Email::mailer(); - $this->mailer = new TestMailer(); - Injector::inst()->registerService($this->mailer, 'Mailer'); - Config::inst()->remove('Email', 'send_all_emails_to'); - // Todo: this could be a special test model $this->model = DataModel::inst(); @@ -289,6 +283,12 @@ class SapphireTest extends PHPUnit_Framework_TestCase { // Clear requirements Requirements::clear(); + + // Set up email + $this->originalMailer = Email::mailer(); + $this->mailer = new TestMailer(); + Injector::inst()->registerService($this->mailer, 'Mailer'); + Config::inst()->remove('Email', 'send_all_emails_to'); } /** diff --git a/security/Member.php b/security/Member.php index 98348c8bb..5426b299f 100644 --- a/security/Member.php +++ b/security/Member.php @@ -865,7 +865,10 @@ class Member extends DataObject implements TemplateGlobalProvider { && $this->record['Password'] && $this->config()->notify_password_change ) { - $e = Member_ChangePasswordEmail::create(); + /** @var Email $e */ + $e = Email::create(); + $e->setSubject(_t('Member.SUBJECTPASSWORDCHANGED', "Your password has been changed", 'Email subject')); + $e->setTemplate('ChangePasswordEmail'); $e->populateTemplate($this); $e->setTo($this->Email); $e->send(); @@ -1762,46 +1765,6 @@ class Member_GroupSet extends ManyManyList { } } -/** - * Class used as template to send an email saying that the password has been - * changed. - * - * @package framework - * @subpackage security - */ -class Member_ChangePasswordEmail extends Email { - - protected $from = ''; // setting a blank from address uses the site's default administrator email - protected $subject = ''; - protected $ss_template = 'ChangePasswordEmail'; - - public function __construct() { - parent::__construct(); - - $this->subject = _t('Member.SUBJECTPASSWORDCHANGED', "Your password has been changed", 'Email subject'); - } -} - - - -/** - * Class used as template to send the forgot password email - * - * @package framework - * @subpackage security - */ -class Member_ForgotPasswordEmail extends Email { - protected $from = ''; // setting a blank from address uses the site's default administrator email - protected $subject = ''; - protected $ss_template = 'ForgotPasswordEmail'; - - public function __construct() { - parent::__construct(); - - $this->subject = _t('Member.SUBJECTPASSWORDRESET', "Your password reset link", 'Email subject'); - } -} - /** * Member Validator * diff --git a/security/MemberLoginForm.php b/security/MemberLoginForm.php index 0169f130a..b451c6baa 100644 --- a/security/MemberLoginForm.php +++ b/security/MemberLoginForm.php @@ -325,7 +325,10 @@ JS; if($member) { $token = $member->generateAutologinTokenAndStoreHash(); - $e = Member_ForgotPasswordEmail::create(); + /** @var Email $e */ + $e = Email::create(); + $e->setSubject(_t('Member.SUBJECTPASSWORDRESET', "Your password reset link", 'Email subject')); + $e->setTemplate('ForgotPasswordEmail'); $e->populateTemplate($member); $e->populateTemplate(array( 'PasswordResetLink' => Security::getPasswordResetLink($member, $token) diff --git a/templates/email/ChangePasswordEmail.ss b/templates/email/ChangePasswordEmail.ss index 2b160c2fd..ee0eeda93 100644 --- a/templates/email/ChangePasswordEmail.ss +++ b/templates/email/ChangePasswordEmail.ss @@ -2,6 +2,6 @@

<%t ChangePasswordEmail_ss.CHANGEPASSWORDTEXT1 'You changed your password for' is 'for a url' %> $AbsoluteBaseURL.
- <%t ChangePasswordEmail_ss.CHANGEPASSWORDFOREMAIL 'The password for account with email address {email} has been changed. If you didn't change your password please change your password using the link below' email=$Email %>
+ <%t ChangePasswordEmail_ss.CHANGEPASSWORDFOREMAIL 'The password for account with email address {email} has been changed. If you didn\'t change your password please change your password using the link below' email=$Email %>
<%t ChangePasswordEmail_ss.CHANGEPASSWORDTEXT3 'Change password' %>

diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php index e5f701721..0ed0a7f56 100644 --- a/tests/security/MemberTest.php +++ b/tests/security/MemberTest.php @@ -185,16 +185,44 @@ class MemberTest extends FunctionalTest { * Test that changed passwords will send an email */ public function testChangedPasswordEmaling() { + $oldSetting = Config::inst()->get('Member', 'notify_password_change'); + Config::inst()->update('Member', 'notify_password_change', true); + $this->clearEmails(); $member = $this->objFromFixture('Member', 'test'); $this->assertNotNull($member); $valid = $member->changePassword('32asDF##$$%%'); $this->assertTrue($valid->valid()); - /* - $this->assertEmailSent("sam@silverstripe.com", null, "/changed password/", - '/sam@silverstripe\.com.*32asDF##\$\$%%/'); - */ + + $this->assertEmailSent("dummy@silverstripe.tld", null, "Your password has been changed", + '/dummy@silverstripe\.tld/'); + + Config::inst()->update('Member', 'notify_password_change', $oldSetting); + } + + /** + * Test that triggering "forgotPassword" sends an Email with a reset link + */ + public function testForgotPasswordEmaling() { + $this->clearEmails(); + $this->autoFollowRedirection = false; + + $member = $this->objFromFixture('Member', 'test'); + $this->assertNotNull($member); + + // Initiate a password-reset + $response = $this->post('Security/LostPasswordForm', array('Email' => $member->Email)); + + $this->assertEquals($response->getStatusCode(), 302); + + // We should get redirected to Security/passwordsent + $this->assertContains('Security/passwordsent/dummy@silverstripe.tld', + urldecode($response->getHeader('Location'))); + + // Check existance of reset link + $this->assertEmailSent("dummy@silverstripe.tld", null, 'Your password reset link', + '/Security\/changepassword\?m='.$member->ID.'&t=[^"]+/'); } /** diff --git a/tests/security/MemberTest.yml b/tests/security/MemberTest.yml index 344325704..38c3e840b 100644 --- a/tests/security/MemberTest.yml +++ b/tests/security/MemberTest.yml @@ -42,7 +42,7 @@ Member: test: FirstName: Test Surname: User - Email: sam@silverstripe.com + Email: dummy@silverstripe.tld Password: 1nitialPassword PasswordExpiry: 2030-01-01 Groups: =>Group.securityadminsgroup diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php index b980db321..c46ab9812 100644 --- a/tests/security/SecurityTest.php +++ b/tests/security/SecurityTest.php @@ -305,13 +305,13 @@ class SecurityTest extends FunctionalTest { */ public function testExpiredPassword() { /* BAD PASSWORDS ARE LOCKED OUT */ - $badResponse = $this->doTestLoginForm('sam@silverstripe.com' , 'badpassword'); + $badResponse = $this->doTestLoginForm('dummy@silverstripe.tld' , 'badpassword'); $this->assertEquals(302, $badResponse->getStatusCode()); $this->assertRegExp('/Security\/login/', $badResponse->getHeader('Location')); $this->assertNull($this->session()->inst_get('loggedInAs')); /* UNEXPIRED PASSWORD GO THROUGH WITHOUT A HITCH */ - $goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); + $goodResponse = $this->doTestLoginForm('dummy@silverstripe.tld' , '1nitialPassword'); $this->assertEquals(302, $goodResponse->getStatusCode()); $this->assertEquals( Controller::join_links(Director::absoluteBaseURL(), 'test/link'), @@ -340,7 +340,7 @@ class SecurityTest extends FunctionalTest { } public function testChangePasswordForLoggedInUsers() { - $goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); + $goodResponse = $this->doTestLoginForm('dummy@silverstripe.tld' , '1nitialPassword'); // Change the password $this->get('Security/changepassword?BackURL=test/back'); @@ -353,7 +353,7 @@ class SecurityTest extends FunctionalTest { $this->assertEquals($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs')); // Check if we can login with the new password - $goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , 'changedPassword'); + $goodResponse = $this->doTestLoginForm('dummy@silverstripe.tld' , 'changedPassword'); $this->assertEquals(302, $goodResponse->getStatusCode()); $this->assertEquals( Controller::join_links(Director::absoluteBaseURL(), 'test/link'), @@ -372,9 +372,9 @@ class SecurityTest extends FunctionalTest { // Request new password by email $response = $this->get('Security/lostpassword'); - $response = $this->post('Security/LostPasswordForm', array('Email' => 'sam@silverstripe.com')); + $response = $this->post('Security/LostPasswordForm', array('Email' => 'dummy@silverstripe.tld')); - $this->assertEmailSent('sam@silverstripe.com'); + $this->assertEmailSent('dummy@silverstripe.tld'); // Load password link from email $admin = DataObject::get_by_id('Member', $admin->ID); @@ -394,7 +394,7 @@ class SecurityTest extends FunctionalTest { $this->assertEquals($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs')); // Check if we can login with the new password - $goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , 'changedPassword'); + $goodResponse = $this->doTestLoginForm('dummy@silverstripe.tld' , 'changedPassword'); $this->assertEquals(302, $goodResponse->getStatusCode()); $this->assertEquals($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs')); @@ -412,7 +412,7 @@ class SecurityTest extends FunctionalTest { // Login with a wrong password for more than the defined threshold for($i = 1; $i <= Member::config()->lock_out_after_incorrect_logins+1; $i++) { - $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); + $this->doTestLoginForm('dummy@silverstripe.tld' , 'incorrectpassword'); $member = DataObject::get_by_id("Member", $this->idFromFixture('Member', 'test')); if($i < Member::config()->lock_out_after_incorrect_logins) { @@ -442,7 +442,7 @@ class SecurityTest extends FunctionalTest { } } - $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); + $this->doTestLoginForm('dummy@silverstripe.tld' , '1nitialPassword'); $this->assertNull( $this->session()->inst_get('loggedInAs'), 'The user can\'t log in after being locked out, even with the right password' @@ -452,7 +452,7 @@ class SecurityTest extends FunctionalTest { $member = DataObject::get_by_id("Member", $this->idFromFixture('Member', 'test')); $member->LockedOutUntil = date('Y-m-d H:i:s', time() - 30); $member->write(); - $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); + $this->doTestLoginForm('dummy@silverstripe.tld' , '1nitialPassword'); $this->assertEquals( $this->session()->inst_get('loggedInAs'), $member->ID, @@ -464,7 +464,7 @@ class SecurityTest extends FunctionalTest { // Login again with wrong password, but less attempts than threshold for($i = 1; $i < Member::config()->lock_out_after_incorrect_logins; $i++) { - $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); + $this->doTestLoginForm('dummy@silverstripe.tld' , 'incorrectpassword'); } $this->assertNull($this->session()->inst_get('loggedInAs')); $this->assertContains( @@ -473,7 +473,7 @@ class SecurityTest extends FunctionalTest { 'The user can retry with a wrong password after the lockout expires' ); - $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); + $this->doTestLoginForm('dummy@silverstripe.tld' , '1nitialPassword'); $this->assertEquals( $this->session()->inst_get('loggedInAs'), $member->ID, @@ -488,8 +488,8 @@ class SecurityTest extends FunctionalTest { // ATTEMPTING LOG-IN TWICE WITH ONE ACCOUNT AND TWICE WITH ANOTHER SHOULDN'T LOCK ANYBODY OUT - $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); - $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); + $this->doTestLoginForm('dummy@silverstripe.tld' , 'incorrectpassword'); + $this->doTestLoginForm('dummy@silverstripe.tld' , 'incorrectpassword'); $this->doTestLoginForm('noexpiry@silverstripe.com' , 'incorrectpassword'); $this->doTestLoginForm('noexpiry@silverstripe.com' , 'incorrectpassword'); @@ -503,7 +503,7 @@ class SecurityTest extends FunctionalTest { // BUT, DOING AN ADDITIONAL LOG-IN WITH EITHER OF THEM WILL LOCK OUT, SINCE THAT IS THE 3RD FAILURE IN // THIS SESSION - $this->doTestLoginForm('sam@silverstripe.com' , 'incorrectpassword'); + $this->doTestLoginForm('dummy@silverstripe.tld' , 'incorrectpassword'); $member1 = DataObject::get_by_id("Member", $this->idFromFixture('Member', 'test')); $this->assertNotNull($member1->LockedOutUntil); @@ -516,16 +516,16 @@ class SecurityTest extends FunctionalTest { Security::config()->login_recording = true; /* UNSUCCESSFUL ATTEMPTS WITH WRONG PASSWORD FOR EXISTING USER ARE LOGGED */ - $this->doTestLoginForm('sam@silverstripe.com', 'wrongpassword'); + $this->doTestLoginForm('dummy@silverstripe.tld', 'wrongpassword'); $attempt = DataObject::get_one('LoginAttempt', array( - '"LoginAttempt"."Email"' => 'sam@silverstripe.com' + '"LoginAttempt"."Email"' => 'dummy@silverstripe.tld' )); $this->assertTrue(is_object($attempt)); $member = DataObject::get_one('Member', array( - '"Member"."Email"' => 'sam@silverstripe.com' + '"Member"."Email"' => 'dummy@silverstripe.tld' )); $this->assertEquals($attempt->Status, 'Failure'); - $this->assertEquals($attempt->Email, 'sam@silverstripe.com'); + $this->assertEquals($attempt->Email, 'dummy@silverstripe.tld'); $this->assertEquals($attempt->MemberID, $member->ID); /* UNSUCCESSFUL ATTEMPTS WITH NONEXISTING USER ARE LOGGED */ @@ -545,16 +545,16 @@ class SecurityTest extends FunctionalTest { Security::config()->login_recording = true; /* SUCCESSFUL ATTEMPTS ARE LOGGED */ - $this->doTestLoginForm('sam@silverstripe.com', '1nitialPassword'); + $this->doTestLoginForm('dummy@silverstripe.tld', '1nitialPassword'); $attempt = DataObject::get_one('LoginAttempt', array( - '"LoginAttempt"."Email"' => 'sam@silverstripe.com' + '"LoginAttempt"."Email"' => 'dummy@silverstripe.tld' )); $member = DataObject::get_one('Member', array( - '"Member"."Email"' => 'sam@silverstripe.com' + '"Member"."Email"' => 'dummy@silverstripe.tld' )); $this->assertTrue(is_object($attempt)); $this->assertEquals($attempt->Status, 'Success'); - $this->assertEquals($attempt->Email, 'sam@silverstripe.com'); + $this->assertEquals($attempt->Email, 'dummy@silverstripe.tld'); $this->assertEquals($attempt->MemberID, $member->ID); }