diff --git a/backup/util/dbops/restore_dbops.class.php b/backup/util/dbops/restore_dbops.class.php index 0911c68f0be..eef73fc1a72 100644 --- a/backup/util/dbops/restore_dbops.class.php +++ b/backup/util/dbops/restore_dbops.class.php @@ -1394,7 +1394,9 @@ abstract class restore_dbops { // Note: for DB deleted users md5(username) is stored *sometimes* in the email field, // hence we are looking there for usernames if not empty. See delete_user() // If match by id and mnethost and user is deleted in DB and - // match by username LIKE 'backup_email.%' or by non empty email = md5(username) => ok, return target user + // match by username LIKE 'substring(backup_email).%' where the substr length matches the retained data in the + // username field (100 - (timestamp + 1) characters), or by non empty email = md5(username) => ok, return target user. + $usernamelookup = core_text::substr($user->email, 0, 89) . '.%'; if ($rec = $DB->get_record_sql("SELECT * FROM {user} u WHERE id = ? @@ -1407,13 +1409,14 @@ abstract class restore_dbops { AND email = ? ) )", - array($user->id, $user->mnethostid, $user->email.'.%', md5($user->username)))) { + array($user->id, $user->mnethostid, $usernamelookup, md5($user->username)))) { return $rec; // Matching user, deleted in DB found, return it } // 1D - Handle users deleted in backup file and "alive" in DB // If match by id and mnethost and user is deleted in backup file - // and match by email = email_without_time(backup_email) => ok, return target user + // and match by substring(email) = email_without_time(backup_email) where the substr length matches the retained data + // in the username field (100 - (timestamp + 1) characters) => ok, return target user. if ($user->deleted) { // Note: for DB deleted users email is stored in username field, hence we // are looking there for emails. See delete_user() @@ -1423,7 +1426,7 @@ abstract class restore_dbops { FROM {user} u WHERE id = ? AND mnethostid = ? - AND UPPER(email) = UPPER(?)", + AND " . $DB->sql_substr('UPPER(email)', 1, 89) . " = UPPER(?)", array($user->id, $user->mnethostid, $trimemail))) { return $rec; // Matching user, deleted in backup file found, return it } @@ -1470,7 +1473,8 @@ abstract class restore_dbops { // Note: for DB deleted users md5(username) is stored *sometimes* in the email field, // hence we are looking there for usernames if not empty. See delete_user() // 2B1 - If match by mnethost and user is deleted in DB and not empty email = md5(username) and - // (by username LIKE 'backup_email.%' or non-zero firstaccess) => ok, return target user + // (by username LIKE 'substring(backup_email).%' or non-zero firstaccess) => ok, return target user. + $usernamelookup = core_text::substr($user->email, 0, 89) . '.%'; if ($rec = $DB->get_record_sql("SELECT * FROM {user} u WHERE mnethostid = ? @@ -1484,14 +1488,15 @@ abstract class restore_dbops { AND firstaccess = ? ) )", - array($user->mnethostid, md5($user->username), $user->email.'.%', $user->firstaccess))) { + array($user->mnethostid, md5($user->username), $usernamelookup, $user->firstaccess))) { return $rec; // Matching user found, return it } // 2B2 - If match by mnethost and user is deleted in DB and - // username LIKE 'backup_email.%' and non-zero firstaccess) => ok, return target user + // username LIKE 'substring(backup_email).%' and non-zero firstaccess) => ok, return target user // (this covers situations where md5(username) wasn't being stored so we require both // the email & non-zero firstaccess to match) + $usernamelookup = core_text::substr($user->email, 0, 89) . '.%'; if ($rec = $DB->get_record_sql("SELECT * FROM {user} u WHERE mnethostid = ? @@ -1499,13 +1504,13 @@ abstract class restore_dbops { AND UPPER(username) LIKE UPPER(?) AND firstaccess != 0 AND firstaccess = ?", - array($user->mnethostid, $user->email.'.%', $user->firstaccess))) { + array($user->mnethostid, $usernamelookup, $user->firstaccess))) { return $rec; // Matching user found, return it } // 2C - Handle users deleted in backup file and "alive" in DB // If match mnethost and user is deleted in backup file - // and match by email = email_without_time(backup_email) and non-zero firstaccess=> ok, return target user + // and match by substring(email) = email_without_time(backup_email) and non-zero firstaccess=> ok, return target user. if ($user->deleted) { // Note: for DB deleted users email is stored in username field, hence we // are looking there for emails. See delete_user() @@ -1514,7 +1519,7 @@ abstract class restore_dbops { if ($rec = $DB->get_record_sql("SELECT * FROM {user} u WHERE mnethostid = ? - AND UPPER(email) = UPPER(?) + AND " . $DB->sql_substr('UPPER(email)', 1, 89) . " = UPPER(?) AND firstaccess != 0 AND firstaccess = ?", array($user->mnethostid, $trimemail, $user->firstaccess))) { diff --git a/backup/util/dbops/tests/restore_dbops_test.php b/backup/util/dbops/tests/restore_dbops_test.php index d0fd2ccfefa..59c127be41c 100644 --- a/backup/util/dbops/tests/restore_dbops_test.php +++ b/backup/util/dbops/tests/restore_dbops_test.php @@ -119,4 +119,246 @@ class restore_dbops_testcase extends advanced_testcase { $this->assertSame('Table "backup_ids_temp" does not exist', $e->getMessage()); } } + + /** + * Data provider for {@link test_precheck_user()} + */ + public function precheck_user_provider() { + + $emailmultiplier = [ + 'shortmail' => 'normalusername@example.com', + 'longmail' => str_repeat('a', 100) // It's not validated, hence any string is ok. + ]; + + $providercases = []; + + foreach ($emailmultiplier as $emailk => $email) { + // Get the related cases. + $cases = $this->precheck_user_cases($email); + // Rename them (keys). + foreach ($cases as $key => $case) { + $providercases[$key . ' - ' . $emailk] = $case; + } + } + + return $providercases; + } + + /** + * Get all the cases implemented in {@link restore_dbops::precheck_users()} + * + * @param string $email + */ + private function precheck_user_cases($email) { + global $CFG; + + $baseuserarr = [ + 'username' => 'normalusername', + 'email' => $email, + 'mnethostid' => $CFG->mnet_localhost_id, + 'firstaccess' => 123456789, + 'deleted' => 0, + 'forceemailcleanup' => false, // Hack to force the DB record to have empty mail. + 'forceduplicateadminallowed' => false]; // Hack to enable import_general_duplicate_admin_allowed. + + return [ + // Cases with samesite = true. + 'samesite match existing (1A)' => [ + 'dbuser' => $baseuserarr, + 'backupuser' => $baseuserarr, + 'samesite' => true, + 'outcome' => 'match' + ], + 'samesite match existing anon (1B)' => [ + 'dbuser' => array_merge($baseuserarr, [ + 'username' => 'anon01']), + 'backupuser' => array_merge($baseuserarr, [ + 'id' => -1, 'username' => 'anon01', 'firstname' => 'anonfirstname01', + 'lastname' => 'anonlastname01', 'email' => 'anon01@doesntexist.invalid']), + 'samesite' => true, + 'outcome' => 'match' + ], + 'samesite match existing deleted in db, alive in backup, by db username (1C)' => [ + 'dbuser' => array_merge($baseuserarr, [ + 'deleted' => 1]), + 'backupuser' => array_merge($baseuserarr, [ + 'username' => 'this_wont_match']), + 'samesite' => true, + 'outcome' => 'match' + ], + 'samesite match existing deleted in db, alive in backup, by db email (1C)' => [ + 'dbuser' => array_merge($baseuserarr, [ + 'deleted' => 1]), + 'backupuser' => array_merge($baseuserarr, [ + 'email' => 'this_wont_match']), + 'samesite' => true, + 'outcome' => 'match' + ], + 'samesite match existing alive in db, deleted in backup (1D)' => [ + 'dbuser' => $baseuserarr, + 'backupuser' => array_merge($baseuserarr, [ + 'deleted' => 1]), + 'samesite' => true, + 'outcome' => 'match' + ], + 'samesite conflict (1E)' => [ + 'dbuser' => $baseuserarr, + 'backupuser' => array_merge($baseuserarr, ['id' => -1]), + 'samesite' => true, + 'outcome' => false + ], + 'samesite create user (1F)' => [ + 'dbuser' => $baseuserarr, + 'backupuser' => array_merge($baseuserarr, [ + 'username' => 'newusername']), + 'samesite' => false, + 'outcome' => true + ], + + // Cases with samesite = false. + 'no samesite match existing, by db email (2A1)' => [ + 'dbuser' => $baseuserarr, + 'backupuser' => array_merge($baseuserarr, [ + 'firstaccess' => 0]), + 'samesite' => false, + 'outcome' => 'match' + ], + 'no samesite match existing, by db firstaccess (2A1)' => [ + 'dbuser' => $baseuserarr, + 'backupuser' => array_merge($baseuserarr, [ + 'email' => 'this_wont_match@example.con']), + 'samesite' => false, + 'outcome' => 'match' + ], + 'no samesite match existing anon (2A1 too)' => [ + 'dbuser' => array_merge($baseuserarr, [ + 'username' => 'anon01']), + 'backupuser' => array_merge($baseuserarr, [ + 'id' => -1, 'username' => 'anon01', 'firstname' => 'anonfirstname01', + 'lastname' => 'anonlastname01', 'email' => 'anon01@doesntexist.invalid']), + 'samesite' => false, + 'outcome' => 'match' + ], + 'no samesite match dupe admin (2A2)' => [ + 'dbuser' => array_merge($baseuserarr, [ + 'username' => 'admin_old_site_id', + 'forceduplicateadminallowed' => true]), + 'backupuser' => array_merge($baseuserarr, [ + 'username' => 'admin']), + 'samesite' => false, + 'outcome' => 'match' + ], + 'no samesite match existing deleted in db, alive in backup, by db username (2B1)' => [ + 'dbuser' => array_merge($baseuserarr, [ + 'deleted' => 1]), + 'backupuser' => array_merge($baseuserarr, [ + 'firstaccess' => 0]), + 'samesite' => false, + 'outcome' => 'match' + ], + 'no samesite match existing deleted in db, alive in backup, by db firstaccess (2B1)' => [ + 'dbuser' => array_merge($baseuserarr, [ + 'deleted' => 1]), + 'backupuser' => array_merge($baseuserarr, [ + 'mail' => 'this_wont_match']), + 'samesite' => false, + 'outcome' => 'match' + ], + 'no samesite match existing deleted in db, alive in backup (2B2)' => [ + 'dbuser' => array_merge($baseuserarr, [ + 'deleted' => 1, + 'forceemailcleanup' => true]), + 'backupuser' => $baseuserarr, + 'samesite' => false, + 'outcome' => 'match' + ], + 'no samesite match existing alive in db, deleted in backup (2C)' => [ + 'dbuser' => $baseuserarr, + 'backupuser' => array_merge($baseuserarr, [ + 'deleted' => 1]), + 'samesite' => false, + 'outcome' => 'match' + ], + 'no samesite conflict (2D)' => [ + 'dbuser' => $baseuserarr, + 'backupuser' => array_merge($baseuserarr, [ + 'email' => 'anotheruser@example.com', 'firstaccess' => 0]), + 'samesite' => false, + 'outcome' => false + ], + 'no samesite create user (2E)' => [ + 'dbuser' => $baseuserarr, + 'backupuser' => array_merge($baseuserarr, [ + 'username' => 'newusername']), + 'samesite' => false, + 'outcome' => true + ], + + ]; + } + + /** + * Test restore precheck_user method + * + * @dataProvider precheck_user_provider + * @covers restore_dbops::precheck_user() + * + * @param array $dbuser + * @param array $backupuser + * @param bool $samesite + * @param mixed $outcome + **/ + public function test_precheck_user($dbuser, $backupuser, $samesite, $outcome) { + global $DB; + + $this->resetAfterTest(); + + $dbuser = (object)$dbuser; + $backupuser = (object)$backupuser; + + $siteid = null; + + // If the backup user must be deleted, simulate it (by temp inserting to DB, deleting and fetching it back). + if ($backupuser->deleted) { + $backupuser->id = $DB->insert_record('user', array_merge((array)$backupuser, ['deleted' => 0])); + delete_user($backupuser); + $backupuser = $DB->get_record('user', ['id' => $backupuser->id]); + $DB->delete_records('user', ['id' => $backupuser->id]); + unset($backupuser->id); + } + + // Create the db user, normally. + $dbuser->id = $DB->insert_record('user', array_merge((array)$dbuser, ['deleted' => 0])); + $backupuser->id = $backupuser->id ?? $dbuser->id; + + // We may want to enable the import_general_duplicate_admin_allowed setting and look for old admin records. + if ($dbuser->forceduplicateadminallowed) { + set_config('import_general_duplicate_admin_allowed', true, 'backup'); + $siteid = 'old_site_id'; + } + + // If the DB user must be deleted, do it and fetch it back. + if ($dbuser->deleted) { + delete_user($dbuser); + // We may want to clean the mail field (old behavior, not containing the current md5(username). + if ($dbuser->forceemailcleanup) { + $DB->set_field('user', 'email', '', ['id' => $dbuser->id]); + } + } + + // Get the dbuser record, because we may have changed it above. + $dbuser = $DB->get_record('user', ['id' => $dbuser->id]); + + $method = (new ReflectionClass('restore_dbops'))->getMethod('precheck_user'); + $method->setAccessible(true); + $result = $method->invoke(null, $backupuser, $samesite, $siteid); + + if (is_bool($result)) { + $this->assertSame($outcome, $result); + } else { + $outcome = $dbuser; // Outcome is not bool, matching found, so it must be the dbuser, + // Just check ids, it means the expected match has been found in database. + $this->assertSame($outcome->id, $result->id); + } + } } diff --git a/lib/moodlelib.php b/lib/moodlelib.php index c068e0e1d98..97f5335b04e 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -4302,7 +4302,13 @@ function delete_user(stdClass $user) { // Generate username from email address, or a fake email. $delemail = !empty($user->email) ? $user->email : $user->username . '.' . $user->id . '@unknownemail.invalid'; - $delname = clean_param($delemail . "." . time(), PARAM_USERNAME); + + $deltime = time(); + $deltimelength = core_text::strlen((string) $deltime); + + // Max username length is 100 chars. Select up to limit - (length of current time + 1 [period character]) from users email. + $delname = clean_param($delemail, PARAM_USERNAME); + $delname = core_text::substr($delname, 0, 100 - ($deltimelength + 1)) . ".{$deltime}"; // Workaround for bulk deletes of users with the same email address. while ($DB->record_exists('user', array('username' => $delname))) { // No need to use mnethostid here. @@ -4317,7 +4323,7 @@ function delete_user(stdClass $user) { $updateuser->email = md5($user->username);// Store hash of username, useful importing/restoring users. $updateuser->idnumber = ''; // Clear this field to free it up. $updateuser->picture = 0; - $updateuser->timemodified = time(); + $updateuser->timemodified = $deltime; // Don't trigger update event, as user is being deleted. user_update_user($updateuser, false, false); diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php index 414007493f9..9922acba646 100644 --- a/lib/tests/moodlelib_test.php +++ b/lib/tests/moodlelib_test.php @@ -2465,6 +2465,59 @@ class core_moodlelib_testcase extends advanced_testcase { $this->resetDebugging(); } + /** + * Test deletion of user with long username + */ + public function test_delete_user_long_username() { + global $DB; + + $this->resetAfterTest(); + + // For users without an e-mail, one will be created during deletion using {$username}.{$id}@unknownemail.invalid format. + $user = $this->getDataGenerator()->create_user([ + 'username' => str_repeat('a', 75), + 'email' => '', + ]); + + delete_user($user); + + // The username for the deleted user shouldn't exceed 100 characters. + $usernamedeleted = $DB->get_field('user', 'username', ['id' => $user->id]); + $this->assertEquals(100, core_text::strlen($usernamedeleted)); + + $timestrlength = core_text::strlen((string) time()); + + // It should start with the user name, and end with the current time. + $this->assertStringStartsWith("{$user->username}.{$user->id}@", $usernamedeleted); + $this->assertRegExp('/\.\d{' . $timestrlength . '}$/', $usernamedeleted); + } + + /** + * Test deletion of user with long email address + */ + public function test_delete_user_long_email() { + global $DB; + + $this->resetAfterTest(); + + // Create user with 90 character email address. + $user = $this->getDataGenerator()->create_user([ + 'email' => str_repeat('a', 78) . '@example.com', + ]); + + delete_user($user); + + // The username for the deleted user shouldn't exceed 100 characters. + $usernamedeleted = $DB->get_field('user', 'username', ['id' => $user->id]); + $this->assertEquals(100, core_text::strlen($usernamedeleted)); + + $timestrlength = core_text::strlen((string) time()); + + // Max username length is 100 chars. Select up to limit - (length of current time + 1 [period character]) from users email. + $expectedemail = core_text::substr($user->email, 0, 100 - ($timestrlength + 1)); + $this->assertRegExp('/^' . preg_quote($expectedemail) . '\.\d{' . $timestrlength . '}$/', $usernamedeleted); + } + /** * Test function convert_to_array() */