From 4477b2132262e2badfefeecb14acb3c28d79448c 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 ac907c45238..14782b45011 100644 --- a/lib/tests/moodlelib_test.php +++ b/lib/tests/moodlelib_test.php @@ -4295,6 +4295,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 + ], ]; } @@ -4305,10 +4326,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 = [ @@ -4317,6 +4343,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) { @@ -4327,7 +4358,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 bdca601d034a34a0fd5aa250f5244445d240c6b7 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 | 3 +++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/lib/moodlelib.php b/lib/moodlelib.php index abbc48d5363..abb32bf47de 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -4694,9 +4694,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) { @@ -4707,7 +4709,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); @@ -4734,8 +4736,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 c1b8e1a23b8..55db34b0098 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -6,6 +6,9 @@ information provided here is intended especially for developers. * Behat timeout constants behat_base::TIMEOUT, EXTENDED_TIMEOUT, and REDUCED_TIMEOUT will be deprecated in 3.7. Please instead use the functions behat_base::get_timeout(), get_extended_timeout(), and get_reduced_timeout(). These allow for timeouts to be increased by a setting in config.php. +* 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.5.5 === From 6abbe519d60cdbb65be414cac6c858a0af33e8ea 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 0503fc7a355109e5d5165c53d44e43c616fe3a7f 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 8927e03f6be..2ca4e47eca1 100644 --- a/login/lib.php +++ b/login/lib.php @@ -355,17 +355,20 @@ 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)) { $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'); } }