From 0c68ae88ebacafcefc0c418bbd7c69d41df1c19b Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Tue, 31 Mar 2020 22:21:48 +0100 Subject: [PATCH] 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 0911c68f0be44..eef73fc1a7207 100644 --- a/backup/util/dbops/restore_dbops.class.php +++ b/backup/util/dbops/restore_dbops.class.php @@ -1394,7 +1394,9 @@ protected static function precheck_user($user, $samesite, $siteid = null) { // 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 @@ protected static function precheck_user($user, $samesite, $siteid = null) { 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 @@ protected static function precheck_user($user, $samesite, $siteid = null) { 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 @@ protected static function precheck_user($user, $samesite, $siteid = null) { // 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 @@ protected static function precheck_user($user, $samesite, $siteid = null) { 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 @@ protected static function precheck_user($user, $samesite, $siteid = null) { 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 @@ protected static function precheck_user($user, $samesite, $siteid = null) { 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 9459580f003ae..59c127be41cf8 100644 --- a/backup/util/dbops/tests/restore_dbops_test.php +++ b/backup/util/dbops/tests/restore_dbops_test.php @@ -127,7 +127,7 @@ public function precheck_user_provider() { $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 @@ public function precheck_user_provider() { /** * 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 @@ private function precheck_user_cases($email) { '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 @@ private function precheck_user_cases($email) { ], 'samesite create user (1F)' => [ 'dbuser' => $baseuserarr, - 'backupuser' => array_merge($baseuserarr,[ + 'backupuser' => array_merge($baseuserarr, [ 'username' => 'newusername']), 'samesite' => false, 'outcome' => true @@ -216,14 +218,14 @@ private function precheck_user_cases($email) { // 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 @@ private function precheck_user_cases($email) { ], '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 @@ private function precheck_user_cases($email) { } /** + * 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 @@ public function test_precheck_user($dbuser, $backupuser, $samesite, $outcome) { // 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]); }