From a7466eb44855db0e17ac30587b4743f8dcaaf7b3 Mon Sep 17 00:00:00 2001 From: Rajesh Taneja Date: Thu, 5 Jun 2014 13:41:20 +0800 Subject: [PATCH] MDL-45641 event: Removed multiple user_updated event trigger user_updated event was being triggred while setting password, which is not correct. It now trigger user_password_updated event. Few more modifications done: 1. Correct event is being triggred. 2. Event is only triggred when password is chnaged. 3. Password is updated via single api. 4. Updated unit test --- lib/moodlelib.php | 40 +++++++++++++++--------------------- lib/tests/moodlelib_test.php | 32 ++++++++++++++++------------- login/change_password.php | 3 --- login/lib.php | 3 --- user/editadvanced.php | 21 ++++++------------- user/upgrade.txt | 7 ------- 6 files changed, 41 insertions(+), 65 deletions(-) delete mode 100644 user/upgrade.txt diff --git a/lib/moodlelib.php b/lib/moodlelib.php index cc480f4db69..9b85c0304d9 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -4710,9 +4710,13 @@ function hash_internal_user_password($password, $fasthash = false) { * * @param stdClass $user User object (password property may be updated). * @param string $password Plain text password. + * @param bool $fasthash If true, use a low cost factor when generating the hash + * This is much faster to generate but makes the hash + * less secure. It is used when lots of hashes need to + * be generated quickly. * @return bool Always returns true. */ -function update_internal_user_password($user, $password) { +function update_internal_user_password($user, $password, $fasthash = false) { global $CFG, $DB; require_once($CFG->libdir.'/password_compat/lib/password.php'); @@ -4721,25 +4725,26 @@ function update_internal_user_password($user, $password) { if ($authplugin->prevent_local_passwords()) { $hashedpassword = AUTH_PASSWORD_NOT_CACHED; } else { - $hashedpassword = hash_internal_user_password($password); + $hashedpassword = hash_internal_user_password($password, $fasthash); } // If verification fails then it means the password has changed. - $passwordchanged = !password_verify($password, $user->password); - $algorithmchanged = password_needs_rehash($user->password, PASSWORD_DEFAULT); + if (isset($user->password)) { + // While creating new user, password in unset in $user object, to avoid + // saving it with user_create() + $passwordchanged = !password_verify($password, $user->password); + $algorithmchanged = password_needs_rehash($user->password, PASSWORD_DEFAULT); + } else { + $passwordchanged = true; + } if ($passwordchanged || $algorithmchanged) { $DB->set_field('user', 'password', $hashedpassword, array('id' => $user->id)); $user->password = $hashedpassword; // Trigger event. - $event = \core\event\user_updated::create(array( - 'objectid' => $user->id, - 'relateduserid' => $user->id, - 'context' => context_user::instance($user->id) - )); - $event->add_record_snapshot('user', $user); - $event->trigger(); + $user = $DB->get_record('user', array('id' => $user->id)); + \core\event\user_password_updated::create_from_user($user)->trigger(); } return true; @@ -5969,18 +5974,7 @@ function setnew_password_and_mail($user, $fasthash = false) { $newpassword = generate_password(); - $hashedpassword = hash_internal_user_password($newpassword, $fasthash); - $DB->set_field('user', 'password', $hashedpassword, array('id' => $user->id)); - $user->password = $hashedpassword; - - // Trigger event. - $event = \core\event\user_updated::create(array( - 'objectid' => $user->id, - 'relateduserid' => $user->id, - 'context' => context_user::instance($user->id) - )); - $event->add_record_snapshot('user', $user); - $event->trigger(); + update_internal_user_password($user, $newpassword, $fasthash); $a = new stdClass(); $a->firstname = fullname($user, true); diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php index 3830bda92e4..50cd1a37c38 100644 --- a/lib/tests/moodlelib_test.php +++ b/lib/tests/moodlelib_test.php @@ -2247,11 +2247,21 @@ class core_moodlelib_testcase extends advanced_testcase { // Manually set the user's password to the md5 of the string 'password'. $DB->set_field('user', 'password', '5f4dcc3b5aa765d61d8327deb882cf99', array('id' => $user->id)); + $sink = $this->redirectEvents(); // Update the password. update_internal_user_password($user, 'password'); + $events = $sink->get_events(); + $sink->close(); + $event = array_pop($events); // Password should have been updated to a bcrypt hash. $this->assertFalse(password_is_legacy_hash($user->password)); + + // Verify event information. + $this->assertInstanceOf('\core\event\user_password_updated', $event); + $this->assertSame($user->id, $event->relateduserid); + $this->assertEquals(context_user::instance($user->id), $event->get_context()); + $this->assertEventContextNotUsed($event); } public function test_fullname() { @@ -2562,9 +2572,9 @@ class core_moodlelib_testcase extends advanced_testcase { } /** - * Test user_updated event trigger by various apis. + * Test setnew_password_and_mail. */ - public function test_user_updated_event() { + public function test_setnew_password_and_mail() { global $DB, $CFG; $this->resetAfterTest(); @@ -2578,27 +2588,21 @@ class core_moodlelib_testcase extends advanced_testcase { $sink = $this->redirectEvents(); $sink2 = $this->redirectEmails(); // Make sure we are redirecting emails. setnew_password_and_mail($user); - update_internal_user_password($user, 'randompass'); $events = $sink->get_events(); $sink->close(); $sink2->close(); + $event = array_pop($events); // Test updated value. $dbuser = $DB->get_record('user', array('id' => $user->id)); $this->assertSame($user->firstname, $dbuser->firstname); - $this->assertNotSame('M00dLe@T', $dbuser->password); + $this->assertNotEmpty($dbuser->password); // Test event. - foreach ($events as $event) { - $this->assertInstanceOf('\core\event\user_updated', $event); - $this->assertSame($user->id, $event->objectid); - $this->assertSame('user_updated', $event->get_legacy_eventname()); - $this->assertEventLegacyData($user, $event); - $this->assertEquals(context_user::instance($user->id), $event->get_context()); - $expectedlogdata = array(SITEID, 'user', 'update', 'view.php?id='.$user->id, ''); - $this->assertEventLegacyLogData($expectedlogdata, $event); - $this->assertEventContextNotUsed($event); - } + $this->assertInstanceOf('\core\event\user_password_updated', $event); + $this->assertSame($user->id, $event->relateduserid); + $this->assertEquals(context_user::instance($user->id), $event->get_context()); + $this->assertEventContextNotUsed($event); } /** diff --git a/login/change_password.php b/login/change_password.php index ba8e8db1d0c..cb433d5c5c4 100644 --- a/login/change_password.php +++ b/login/change_password.php @@ -122,9 +122,6 @@ if ($mform->is_cancelled()) { unset_user_preference('auth_forcepasswordchange', $USER); unset_user_preference('create_password', $USER); - $user = $DB->get_record('user', array('id' => $USER->id), '*', MUST_EXIST); - \core\event\user_password_updated::create_from_user($user)->trigger(); - $strpasswordchanged = get_string('passwordchanged'); $fullname = fullname($USER, true); diff --git a/login/lib.php b/login/lib.php index d0b8f5d2c34..dd357f463bb 100644 --- a/login/lib.php +++ b/login/lib.php @@ -251,9 +251,6 @@ function core_login_process_password_set($token) { } complete_user_login($user); // Triggers the login event. - $user = $DB->get_record('user', array('id' => $user->id), '*', MUST_EXIST); - \core\event\user_password_updated::create_from_user($user, true)->trigger(); - $urltogo = core_login_get_return_url(); unset($SESSION->wantsurl); redirect($urltogo, get_string('passwordset'), 1); diff --git a/user/editadvanced.php b/user/editadvanced.php index 2cf36c89a90..d363758cf8d 100644 --- a/user/editadvanced.php +++ b/user/editadvanced.php @@ -167,7 +167,6 @@ if ($usernew = $userform->get_data()) { $usernew->timemodified = time(); $createpassword = false; - $passwordupdated = false; if ($usernew->id == -1) { unset($usernew->id); @@ -192,8 +191,6 @@ if ($usernew = $userform->get_data()) { if (!$authplugin->user_update_password($usernew, $usernew->newpassword)) { // Do not stop here, we need to finish user creation. debugging(get_string('cannotupdatepasswordonextauth', '', '', $usernew->auth), DEBUG_NONE); - } else { - $passwordupdated = true; } } $usercreated = true; @@ -211,8 +208,6 @@ if ($usernew = $userform->get_data()) { if ($authplugin->can_change_password()) { if (!$authplugin->user_update_password($usernew, $usernew->newpassword)) { print_error('cannotupdatepasswordonextauth', '', '', $usernew->auth); - } else { - $passwordupdated = true; } unset_user_preference('create_password', $usernew); // Prevent cron from generating the password. } @@ -251,6 +246,12 @@ if ($usernew = $userform->get_data()) { // Reload from db. $usernew = $DB->get_record('user', array('id' => $usernew->id)); + if ($createpassword) { + setnew_password_and_mail($usernew); + unset_user_preference('create_password', $usernew); + set_user_preference('auth_forcepasswordchange', 1, $usernew); + } + // Trigger update/create event, after all fields are stored. if ($usercreated) { \core\event\user_created::create_from_userid($usernew->id)->trigger(); @@ -258,16 +259,6 @@ if ($usernew = $userform->get_data()) { \core\event\user_updated::create_from_userid($usernew->id)->trigger(); } - if ($passwordupdated) { - \core\event\user_password_updated::create_from_user($usernew)->trigger(); - } - - if ($createpassword) { - setnew_password_and_mail($usernew); - unset_user_preference('create_password', $usernew); - set_user_preference('auth_forcepasswordchange', 1, $usernew); - } - if ($user->id == $USER->id) { // Override old $USER session variable. foreach ((array)$usernew as $variable => $value) { diff --git a/user/upgrade.txt b/user/upgrade.txt deleted file mode 100644 index 9ecf4220a5d..00000000000 --- a/user/upgrade.txt +++ /dev/null @@ -1,7 +0,0 @@ -This files describes API changes in /user/, information provided -here is intended especially for developers. - -=== 2.7.1 === - -* user_update_user() and user_create_user() api's accept optional param - $triggerevent to avoid respective events to be triggred from the api's.