From 7f90fb5faf273529963eed181bbd23f56096a0d2 Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Wed, 6 Mar 2019 14:49:34 +0800 Subject: [PATCH 1/4] MDL-29318 core: More unit tests for get_complete_user_data() --- lib/tests/moodlelib_test.php | 43 ++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php index 199f3f10f36..b6e72410c92 100644 --- a/lib/tests/moodlelib_test.php +++ b/lib/tests/moodlelib_test.php @@ -4313,6 +4313,27 @@ class core_moodlelib_testcase extends advanced_testcase { 'Fetch data using an invalid username' => [ 'username', 's2', false ], + 'Fetch by email' => [ + 'email', 's1@example.com', true + ], + 'Fetch data using a non-existent email' => [ + 'email', 's2@example.com', false + ], + 'Fetch data using a non-existent email, throw exception' => [ + 'email', 's2@example.com', false, dml_missing_record_exception::class + ], + 'Multiple accounts with the same email' => [ + 'email', 's1@example.com', false, 1 + ], + 'Multiple accounts with the same email, throw exception' => [ + 'email', 's1@example.com', false, 1, dml_multiple_records_exception::class + ], + 'Fetch data using a valid user ID' => [ + 'id', true, true + ], + 'Fetch data using a non-existent user ID' => [ + 'id', false, false + ], ]; } @@ -4323,10 +4344,15 @@ class core_moodlelib_testcase extends advanced_testcase { * @param string $field The field to use for the query. * @param string|boolean $value The field value. When fetching by ID, set true to fetch valid user ID, false otherwise. * @param boolean $success Whether we expect for the fetch to succeed or return false. + * @param int $allowaccountssameemail Value for $CFG->allowaccountssameemail. + * @param string $expectedexception The exception to be expected. */ - public function test_get_complete_user_data($field, $value, $success) { + public function test_get_complete_user_data($field, $value, $success, $allowaccountssameemail = 0, $expectedexception = '') { $this->resetAfterTest(); + // Set config settings we need for our environment. + set_config('allowaccountssameemail', $allowaccountssameemail); + // Generate the user data. $generator = $this->getDataGenerator(); $userdata = [ @@ -4335,6 +4361,11 @@ class core_moodlelib_testcase extends advanced_testcase { ]; $user = $generator->create_user($userdata); + if ($allowaccountssameemail) { + // Create another user with the same email address. + $generator->create_user(['email' => 's1@example.com']); + } + // Since the data provider can't know what user ID to use, do a special handling for ID field tests. if ($field === 'id') { if ($value) { @@ -4345,7 +4376,15 @@ class core_moodlelib_testcase extends advanced_testcase { $value = $user->id + 1; } } - $fetcheduser = get_complete_user_data($field, $value); + + // When an exception is expected. + $throwexception = false; + if ($expectedexception) { + $this->expectException($expectedexception); + $throwexception = true; + } + + $fetcheduser = get_complete_user_data($field, $value, null, $throwexception); if ($success) { $this->assertEquals($user->id, $fetcheduser->id); $this->assertEquals($user->username, $fetcheduser->username); From b858b161309d2e0fdf7c20a7564df76c658bdbe7 Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Wed, 6 Mar 2019 14:55:42 +0800 Subject: [PATCH 2/4] MDL-29318 core: Fixes for get_complete_user_data() * Added email in the list of case-insensitive fields. * New optional parameter $throwexception for \get_complete_user_data(). If true, an exception will be thrown when there's no matching record found or when there are multiple records found for the given field value. If false, it will simply return false. Defaults to false when not set. This ensures that get_complete_user_data() fetches the correct user data. --- lib/moodlelib.php | 20 ++++++++++++++++---- lib/upgrade.txt | 7 +++++-- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/lib/moodlelib.php b/lib/moodlelib.php index fc88a137b89..14965cf616a 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -4767,9 +4767,11 @@ function update_internal_user_password($user, $password, $fasthash = false) { * @param string $field The user field to be checked for a given value. * @param string $value The value to match for $field. * @param int $mnethostid + * @param bool $throwexception If true, it will throw an exception when there's no record found or when there are multiple records + * found. Otherwise, it will just return false. * @return mixed False, or A {@link $USER} object. */ -function get_complete_user_data($field, $value, $mnethostid = null) { +function get_complete_user_data($field, $value, $mnethostid = null, $throwexception = false) { global $CFG, $DB; if (!$field || !$value) { @@ -4780,7 +4782,7 @@ function get_complete_user_data($field, $value, $mnethostid = null) { $field = core_text::strtolower($field); // List of case insensitive fields. - $caseinsensitivefields = ['username']; + $caseinsensitivefields = ['username', 'email']; // Build the WHERE clause for an SQL query. $params = array('fieldval' => $value); @@ -4807,8 +4809,18 @@ function get_complete_user_data($field, $value, $mnethostid = null) { } // Get all the basic user data. - if (! $user = $DB->get_record_select('user', $constraints, $params)) { - return false; + try { + // Make sure that there's only a single record that matches our query. + // For example, when fetching by email, multiple records might match the query as there's no guarantee that email addresses + // are unique. Therefore we can't reliably tell whether the user profile data that we're fetching is the correct one. + $user = $DB->get_record_select('user', $constraints, $params, '*', MUST_EXIST); + } catch (dml_exception $exception) { + if ($throwexception) { + throw $exception; + } else { + // Return false when no records or multiple records were found. + return false; + } } // Get various settings and preferences. diff --git a/lib/upgrade.txt b/lib/upgrade.txt index ab0d4e82b94..75ec1c4d42f 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -26,6 +26,9 @@ attribute on forms to avoid collisions in forms loaded in AJAX requests. When the parameter is set to that constant, the function won't process file merging, keeping the original state of the file area. * Introduced new callback for plugin developers '_pre_processor_message_send($procname, $proceventdata)': This will allow any plugin to manipulate messages or notifications before they are sent by a processor (email, mobile...) +* New optional parameter $throwexception for \get_complete_user_data(). If true, an exception will be thrown when there's no + matching record found or when there are multiple records found for the given field value. If false, it will simply return false. + Defaults to false when not set. === 3.6 === @@ -641,13 +644,13 @@ the groupid field. $OUTPUT->download_dataformat_selector() instead. when building Xpath, or pass the unescaped value when using the named selector. * Add new file_is_executable(), to consistently check for executables even in Windows (PHP bug #41062). -* Introduced new callbacks for plugin developers. +* Introduced new hooks for plugin developers. - _pre_course_category_delete($category) - _pre_course_delete($course) - _pre_course_module_delete($cm) - _pre_block_delete($instance) - _pre_user_delete($user) - These callbacks allow developers to use the item in question before it is deleted by core. For example, if your plugin is + These hooks allow developers to use the item in question before it is deleted by core. For example, if your plugin is a module (plugins located in the mod folder) called 'xxx' and you wish to interact with the user object before it is deleted then the function to create would be mod_xxx_pre_user_delete($user) in mod/xxx/lib.php. * pear::Net::GeoIP has been removed. From 99ead77b1cc1a8585d8a78d05872969c99ccec43 Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Wed, 6 Mar 2019 14:57:17 +0800 Subject: [PATCH 3/4] MDL-29318 login: Additional test for forgot_password_data_provider() --- login/tests/lib_test.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/login/tests/lib_test.php b/login/tests/lib_test.php index fccb928e751..160d9def977 100644 --- a/login/tests/lib_test.php +++ b/login/tests/lib_test.php @@ -271,6 +271,11 @@ class core_login_lib_testcase extends advanced_testcase { ['email' => get_string('forgottenduplicate')], ['allowaccountssameemail' => 1] ], + 'Multiple accounts with the same email but with different case' => [ + ['email' => 'S1@EXAMPLE.COM'], + ['email' => get_string('forgottenduplicate')], + ['allowaccountssameemail' => 1] + ], 'Non-existent email, username protection on' => [ ['email' => 's2@example.com'] ], From e9e1ecafda16b8b23547ab5173b204b526f8a529 Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Wed, 6 Mar 2019 15:34:38 +0800 Subject: [PATCH 4/4] MDL-29318 login: Handle email in case-insensitive manner * Let get_complete_user_data() handle the fetching of user data and handle the logic of the errors to be shown based on the exception it throws. This also saves us 1 DB query by eliminating the need to count for the users that match a given email first before fetching user information. --- login/lib.php | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/login/lib.php b/login/lib.php index eb29ae1fcac..42643335021 100644 --- a/login/lib.php +++ b/login/lib.php @@ -355,18 +355,21 @@ function core_login_validate_forgot_password_data($data) { if (!validate_email($data['email'])) { $errors['email'] = get_string('invalidemail'); - } else if ($DB->count_records('user', array('email' => $data['email'])) > 1) { - $errors['email'] = get_string('forgottenduplicate'); - } else { - if ($user = get_complete_user_data('email', $data['email'])) { + try { + $user = get_complete_user_data('email', $data['email'], null, true); if (empty($user->confirmed)) { send_confirmation_email($user); $errors['email'] = get_string('confirmednot'); } - } - if (!$user and empty($CFG->protectusernames)) { - $errors['email'] = get_string('emailnotfound'); + } catch (dml_missing_record_exception $missingexception) { + // User not found. Show error when $CFG->protectusernames is turned off. + if (empty($CFG->protectusernames)) { + $errors['email'] = get_string('emailnotfound'); + } + } catch (dml_multiple_records_exception $multipleexception) { + // Multiple records found. Ask the user to enter a username instead. + $errors['email'] = get_string('forgottenduplicate'); } }