From 2c6df5b7f0bad6858f0e5ac23ea3079e5e519085 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Tue, 31 Dec 2019 13:18:29 +0000 Subject: [PATCH 1/3] MDL-67499 user: truncate long username/email during user deletion. --- lib/moodlelib.php | 10 +++++-- lib/tests/moodlelib_test.php | 53 ++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/lib/moodlelib.php b/lib/moodlelib.php index f32e8a2ab21..36e1af1d0c5 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -4274,7 +4274,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. @@ -4289,7 +4295,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 d76ca146be2..99eeea504bf 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() */ From a7c8680ec34c057d7cb4fe1cdd7870ea0dd9a4ff Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Sat, 28 Mar 2020 19:53:36 +0100 Subject: [PATCH 2/3] MDL-67499 restore: add 100% cov. to restore_dbops::precheck_user() Supports multiple mails to be tested. So we can perform changes to it and also some related stuff, like delete_user() with confidence. --- .../util/dbops/tests/restore_dbops_test.php | 233 ++++++++++++++++++ 1 file changed, 233 insertions(+) diff --git a/backup/util/dbops/tests/restore_dbops_test.php b/backup/util/dbops/tests/restore_dbops_test.php index d0fd2ccfefa..9459580f003 100644 --- a/backup/util/dbops/tests/restore_dbops_test.php +++ b/backup/util/dbops/tests/restore_dbops_test.php @@ -119,4 +119,237 @@ 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()} + */ + 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 + ], + + ]; + } + + /** + * @dataProvider precheck_user_provider + * @covers restore_dbops::precheck_user() + * */ + 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); + } + } } From 23e71ce7a5a89dbfd55469f08d38855ef8a22707 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Tue, 31 Mar 2020 22:21:48 +0100 Subject: [PATCH 3/3] MDL-67499 restore: account for deleted user truncated email/username. --- backup/util/dbops/restore_dbops.class.php | 25 ++++++++++------- .../util/dbops/tests/restore_dbops_test.php | 27 ++++++++++++------- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/backup/util/dbops/restore_dbops.class.php b/backup/util/dbops/restore_dbops.class.php index 7ef9117e230..f784b82f7c5 100644 --- a/backup/util/dbops/restore_dbops.class.php +++ b/backup/util/dbops/restore_dbops.class.php @@ -1371,7 +1371,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 = ? @@ -1384,13 +1386,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() @@ -1400,7 +1403,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 } @@ -1447,7 +1450,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 = ? @@ -1461,14 +1465,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 = ? @@ -1476,13 +1481,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() @@ -1491,7 +1496,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 9459580f003..59c127be41c 100644 --- a/backup/util/dbops/tests/restore_dbops_test.php +++ b/backup/util/dbops/tests/restore_dbops_test.php @@ -127,7 +127,7 @@ class restore_dbops_testcase extends advanced_testcase { $emailmultiplier = [ 'shortmail' => 'normalusername@example.com', - //'longmail' => str_repeat('a', 100) // It's not validated, hence any string is ok. + 'longmail' => str_repeat('a', 100) // It's not validated, hence any string is ok. ]; $providercases = []; @@ -146,6 +146,8 @@ class restore_dbops_testcase extends advanced_testcase { /** * Get all the cases implemented in {@link restore_dbops::precheck_users()} + * + * @param string $email */ private function precheck_user_cases($email) { global $CFG; @@ -154,7 +156,7 @@ class restore_dbops_testcase extends advanced_testcase { 'username' => 'normalusername', 'email' => $email, 'mnethostid' => $CFG->mnet_localhost_id, - 'firstaccess'=> 123456789, + '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. @@ -207,7 +209,7 @@ class restore_dbops_testcase extends advanced_testcase { ], 'samesite create user (1F)' => [ 'dbuser' => $baseuserarr, - 'backupuser' => array_merge($baseuserarr,[ + 'backupuser' => array_merge($baseuserarr, [ 'username' => 'newusername']), 'samesite' => false, 'outcome' => true @@ -216,14 +218,14 @@ class restore_dbops_testcase extends advanced_testcase { // Cases with samesite = false. 'no samesite match existing, by db email (2A1)' => [ 'dbuser' => $baseuserarr, - 'backupuser' => array_merge($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,[ + 'backupuser' => array_merge($baseuserarr, [ 'email' => 'this_wont_match@example.con']), 'samesite' => false, 'outcome' => 'match' @@ -279,14 +281,14 @@ class restore_dbops_testcase extends advanced_testcase { ], 'no samesite conflict (2D)' => [ 'dbuser' => $baseuserarr, - 'backupuser' => array_merge($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,[ + 'backupuser' => array_merge($baseuserarr, [ 'username' => 'newusername']), 'samesite' => false, 'outcome' => true @@ -296,9 +298,16 @@ class restore_dbops_testcase extends advanced_testcase { } /** + * 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; @@ -331,7 +340,7 @@ class restore_dbops_testcase extends advanced_testcase { // 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) + // 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]); }