MDL-68183 auth: Fix the performance of signup_validate_data search query
authorDavid Mudrák <david@moodle.com>
Tue, 17 Mar 2020 12:04:07 +0000 (13:04 +0100)
committerDavid Mudrák <david@moodle.com>
Tue, 17 Mar 2020 13:52:32 +0000 (14:52 +0100)
When searching for other users with the same email address, we perform
the case-insensitive and accent-sensitive search. That may be expensive
as some DBs such as MySQL cannot use the index in that case. Instead,
sequential scan of all the user records is performed and the comparison
uses the LOWER function to filter the matching records. This leads to
significant performance heavy queries which in turn represent a surface
for DoS attacks.

For that reason, we first perform accent-insensitive search for
potential candidates in a subselect, which can use the index. Only then
we perform the additional accent-sensitive search on this limited set or
records.

lib/authlib.php
lib/tests/authlib_test.php

index 1d2f972..d1604e6 100644 (file)
@@ -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);
index f0a3d67..99f5923 100644 (file)
@@ -414,4 +414,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;
+    }
 }