diff --git a/lib/authlib.php b/lib/authlib.php index 1d2f972e912..d1604e6a18a 100644 --- a/lib/authlib.php +++ b/lib/authlib.php @@ -1027,14 +1027,25 @@ function signup_validate_data($data, $files) { $errors['email'] = get_string('invalidemail'); } else if (empty($CFG->allowaccountssameemail)) { - // Make a case-insensitive query for the given email address. - $select = $DB->sql_equal('email', ':email', false) . ' AND mnethostid = :mnethostid'; + // Emails in Moodle as case-insensitive and accents-sensitive. Such a combination can lead to very slow queries + // on some DBs such as MySQL. So we first get the list of candidate users in a subselect via more effective + // accent-insensitive query that can make use of the index and only then we search within that limited subset. + $sql = "SELECT 'x' + FROM {user} + WHERE " . $DB->sql_equal('email', ':email1', false, true) . " + AND id IN (SELECT id + FROM {user} + WHERE " . $DB->sql_equal('email', ':email2', false, false) . " + AND mnethostid = :mnethostid)"; + $params = array( - 'email' => $data['email'], + 'email1' => $data['email'], + 'email2' => $data['email'], 'mnethostid' => $CFG->mnet_localhost_id, ); + // If there are other user(s) that already have the same email, show an error. - if ($DB->record_exists_select('user', $select, $params)) { + if ($DB->record_exists_sql($sql, $params)) { $forgotpasswordurl = new moodle_url('/login/forgot_password.php'); $forgotpasswordlink = html_writer::link($forgotpasswordurl, get_string('emailexistshintlink')); $errors['email'] = get_string('emailexists') . ' ' . get_string('emailexistssignuphint', 'moodle', $forgotpasswordlink); diff --git a/lib/moodlelib.php b/lib/moodlelib.php index 71dcf51dac9..e5ee465dc53 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -4877,11 +4877,15 @@ function get_complete_user_data($field, $value, $mnethostid = null, $throwexcept // Build the WHERE clause for an SQL query. $params = array('fieldval' => $value); - // Do a case-insensitive query, if necessary. + // Do a case-insensitive query, if necessary. These are generally very expensive. The performance can be improved on some DBs + // such as MySQL by pre-filtering users with accent-insensitive subselect. if (in_array($field, $caseinsensitivefields)) { $fieldselect = $DB->sql_equal($field, ':fieldval', false); + $idsubselect = $DB->sql_equal($field, ':fieldval2', false, false); + $params['fieldval2'] = $value; } else { $fieldselect = "$field = :fieldval"; + $idsubselect = ''; } $constraints = "$fieldselect AND deleted <> 1"; @@ -4898,6 +4902,10 @@ function get_complete_user_data($field, $value, $mnethostid = null, $throwexcept $constraints .= " AND mnethostid = :mnethostid"; } + if ($idsubselect) { + $constraints .= " AND id IN (SELECT id FROM {user} WHERE {$idsubselect})"; + } + // Get all the basic user data. try { // Make sure that there's only a single record that matches our query. diff --git a/lib/tests/authlib_test.php b/lib/tests/authlib_test.php index e59e3833296..ffa7128bc06 100644 --- a/lib/tests/authlib_test.php +++ b/lib/tests/authlib_test.php @@ -345,4 +345,62 @@ class core_authlib_testcase extends advanced_testcase { $this->assertInstanceOf('coding_exception', $e); } } + + /** + * Test the {@link signup_validate_data()} duplicate email validation. + */ + public function test_signup_validate_data_same_email() { + global $CFG; + require_once($CFG->libdir . '/authlib.php'); + require_once($CFG->dirroot . '/user/profile/lib.php'); + + $this->resetAfterTest(); + + $CFG->registerauth = 'email'; + $CFG->passwordpolicy = false; + + // In this test, we want to check accent-sensitive email search. However, accented email addresses do not pass + // the default `validate_email()` and Moodle does not yet provide a CFG switch to allow such emails. So we + // inject our own validation method here and revert it back once we are done. This custom validator method is + // identical to the default 'php' validator with the only difference: it has the FILTER_FLAG_EMAIL_UNICODE set + // so that it allows to use non-ASCII characters in email addresses. + $defaultvalidator = moodle_phpmailer::$validator; moodle_phpmailer::$validator = function($address) { + return (bool) filter_var($address, FILTER_VALIDATE_EMAIL, FILTER_FLAG_EMAIL_UNICODE); + }; + + // Check that two users cannot share the same email address if the site is configured so. + // Emails in Moodle are supposed to be case-insensitive (and accent-sensitive but accents are not yet supported). + $CFG->allowaccountssameemail = false; + + $u1 = $this->getDataGenerator()->create_user([ + 'username' => 'abcdef', + 'email' => 'abcdef@example.com', + ]); + + $formdata = [ + 'username' => 'newuser', + 'firstname' => 'First', + 'lastname' => 'Last', + 'password' => 'weak', + 'email' => 'ABCDEF@example.com', + ]; + + $errors = signup_validate_data($formdata, []); + $this->assertContains('This email address is already registered.', $errors['email']); + + // Emails are accent-sensitive though so if we change a -> á in the u1's email, it should pass. + // Please note that Moodle does not normally support such emails yet. We test the DB search sensitivity here. + $formdata['email'] = 'ábcdef@example.com'; + $errors = signup_validate_data($formdata, []); + $this->assertArrayNotHasKey('email', $errors); + + // Check that users can share the same email if the site is configured so. + $CFG->allowaccountssameemail = true; + $formdata['email'] = 'abcdef@example.com'; + $errors = signup_validate_data($formdata, []); + $this->assertArrayNotHasKey('email', $errors); + + // Restore the original email address validator. + moodle_phpmailer::$validator = $defaultvalidator; + } } diff --git a/login/lib.php b/login/lib.php index b391ea7b712..e0644050bdb 100644 --- a/login/lib.php +++ b/login/lib.php @@ -24,6 +24,9 @@ * @copyright Peter Bulmer * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ + +defined('MOODLE_INTERNAL') || die(); + define('PWRESET_STATUS_NOEMAILSENT', 1); define('PWRESET_STATUS_TOKENSENT', 2); define('PWRESET_STATUS_OTHEREMAILSENT', 3); @@ -93,14 +96,31 @@ function core_login_process_password_reset($username, $email) { $user = $DB->get_record('user', $userparams); } else { // Try to load the user record based on email address. - // this is tricky because + // This is tricky because: // 1/ the email is not guaranteed to be unique - TODO: send email with all usernames to select the account for pw reset // 2/ mailbox may be case sensitive, the email domain is case insensitive - let's pretend it is all case-insensitive. + // + // The case-insensitive + accent-sensitive search may be expensive as some DBs such as MySQL cannot use the + // index in that case. For that reason, we first perform accent-insensitive search in a subselect for potential + // candidates (which can use the index) and only then perform the additional accent-sensitive search on this + // limited set of records in the outer select. + $sql = "SELECT * + FROM {user} + WHERE " . $DB->sql_equal('email', ':email1', false, true) . " + AND id IN (SELECT id + FROM {user} + WHERE mnethostid = :mnethostid + AND deleted = 0 + AND suspended = 0 + AND " . $DB->sql_equal('email', ':email2', false, false) . ")"; - $select = $DB->sql_like('email', ':email', false, true, false, '|') . - " AND mnethostid = :mnethostid AND deleted=0 AND suspended=0"; - $params = array('email' => $DB->sql_like_escape($email, '|'), 'mnethostid' => $CFG->mnet_localhost_id); - $user = $DB->get_record_select('user', $select, $params, '*', IGNORE_MULTIPLE); + $params = array( + 'email1' => $email, + 'email2' => $email, + 'mnethostid' => $CFG->mnet_localhost_id, + ); + + $user = $DB->get_record_sql($sql, $params, IGNORE_MULTIPLE); } // Target user details have now been identified, or we know that there is no such account. diff --git a/login/tests/lib_test.php b/login/tests/lib_test.php index a32941c6390..02bc226104f 100644 --- a/login/tests/lib_test.php +++ b/login/tests/lib_test.php @@ -355,4 +355,75 @@ class core_login_lib_testcase extends advanced_testcase { $this->assertArrayNotHasKey('email', $validationerrors); } } + + /** + * Test searching for the user record by matching the provided email address when resetting password. + * + * Email addresses should be handled as case-insensitive but accent sensitive. + */ + public function test_core_login_process_password_reset_email_sensitivity() { + global $CFG; + require_once($CFG->libdir.'/phpmailer/moodle_phpmailer.php'); + + $this->resetAfterTest(); + $sink = $this->redirectEmails(); + $CFG->protectusernames = 0; + + // In this test, we need to mock sending emails on non-ASCII email addresses. However, such email addresses do + // not pass the default `validate_email()` and Moodle does not yet provide a CFG switch to allow such emails. + // So we inject our own validation method here and revert it back once we are done. This custom validator method + // is identical to the default 'php' validator with the only difference: it has the FILTER_FLAG_EMAIL_UNICODE + // set so that it allows to use non-ASCII characters in email addresses. + $defaultvalidator = moodle_phpmailer::$validator; + moodle_phpmailer::$validator = function($address) { + return (bool) filter_var($address, FILTER_VALIDATE_EMAIL, FILTER_FLAG_EMAIL_UNICODE); + }; + + // Emails are treated as case-insensitive when searching for the matching user account. + $u1 = $this->getDataGenerator()->create_user(['email' => 'priliszlutouckykunupeldabelskeody@example.com']); + + list($status, $notice, $url) = core_login_process_password_reset(null, 'PrIlIsZlUtOuCkYKuNupELdAbElSkEoDy@eXaMpLe.CoM'); + + $this->assertSame('emailresetconfirmsent', $status); + $emails = $sink->get_messages(); + $this->assertCount(1, $emails); + $email = reset($emails); + $this->assertSame($u1->email, $email->to); + $sink->clear(); + + // There may exist two users with same emails. + $u2 = $this->getDataGenerator()->create_user(['email' => 'PRILISZLUTOUCKYKUNUPELDABELSKEODY@example.com']); + + list($status, $notice, $url) = core_login_process_password_reset(null, 'PrIlIsZlUtOuCkYKuNupELdAbElSkEoDy@eXaMpLe.CoM'); + + $this->assertSame('emailresetconfirmsent', $status); + $emails = $sink->get_messages(); + $this->assertCount(1, $emails); + $email = reset($emails); + $this->assertSame(core_text::strtolower($u2->email), core_text::strtolower($email->to)); + $sink->clear(); + + // However, emails are accent sensitive - note this is the u1's email with a single character a -> á changed. + list($status, $notice, $url) = core_login_process_password_reset(null, 'priliszlutouckykunupeldábelskeody@example.com'); + + $this->assertSame('emailpasswordconfirmnotsent', $status); + $emails = $sink->get_messages(); + $this->assertCount(0, $emails); + $sink->clear(); + + $u3 = $this->getDataGenerator()->create_user(['email' => 'PřílišŽluťoučkýKůňÚpělĎálebskéÓdy@example.com']); + + list($status, $notice, $url) = core_login_process_password_reset(null, 'pŘÍLIŠžLuŤOuČkÝkŮŇúPĚLďÁLEBSKÉóDY@eXaMpLe.CoM'); + + $this->assertSame('emailresetconfirmsent', $status); + $emails = $sink->get_messages(); + $this->assertCount(1, $emails); + $email = reset($emails); + $this->assertSame($u3->email, $email->to); + $sink->clear(); + + // Restore the original email address validator. + moodle_phpmailer::$validator = $defaultvalidator; + } + }