From 5e60be8aafa664bda543318943d0a025595c73a8 Mon Sep 17 00:00:00 2001 From: Simey Lameze Date: Mon, 18 Apr 2016 09:02:41 +0800 Subject: [PATCH] MDL-52781 auth_db: deprecate clean_data method. The old clean_data method has been deprecated as the user_create_user and user_updated user will be responsible by validating the user data. --- auth/db/auth.php | 24 +++-------------- auth/db/tests/db_test.php | 57 +++++++++++---------------------------- 2 files changed, 19 insertions(+), 62 deletions(-) diff --git a/auth/db/auth.php b/auth/db/auth.php index 8c2428de61f17..ebebaa169fc2a 100644 --- a/auth/db/auth.php +++ b/auth/db/auth.php @@ -328,7 +328,6 @@ function sync_users(progress_trace $trace, $do_updates=false) { $updateuser = new stdClass(); $updateuser->id = $user->id; $updateuser->suspended = 1; - $updateuser = $this->clean_data($updateuser); user_update_user($updateuser, false); $trace->output(get_string('auth_dbsuspenduser', 'auth_db', array('name'=>$user->username, 'id'=>$user->id)), 1); } @@ -415,7 +414,6 @@ function sync_users(progress_trace $trace, $do_updates=false) { $updateuser = new stdClass(); $updateuser->id = $olduser->id; $updateuser->suspended = 0; - $updateuser = $this->clean_data($updateuser); user_update_user($updateuser); $trace->output(get_string('auth_dbreviveduser', 'auth_db', array('name' => $username, 'id' => $olduser->id)), 1); @@ -438,7 +436,6 @@ function sync_users(progress_trace $trace, $do_updates=false) { $trace->output(get_string('auth_dbinsertuserduplicate', 'auth_db', array('username'=>$user->username, 'auth'=>$collision->auth)), 1); continue; } - $user = $this->clean_data($user); try { $id = user_create_user($user, false); // It is truly a new user. $trace->output(get_string('auth_dbinsertuser', 'auth_db', array('name'=>$user->username, 'id'=>$id)), 1); @@ -580,7 +577,6 @@ function update_user_record($username, $updatekeys=false) { } if ($needsupdate) { require_once($CFG->dirroot . '/user/lib.php'); - $updateuser = $this->clean_data($updateuser); user_update_user($updateuser); } return $DB->get_record('user', array('id'=>$userid, 'deleted'=>0)); @@ -913,26 +909,14 @@ public function test_settings() { /** * Clean the user data that comes from an external database. - * + * @deprecated since 3.1, please use core_user::clean_data() instead. * @param array $user the user data to be validated against properties definition. * @return stdClass $user the cleaned user data. */ public function clean_data($user) { - if (empty($user)) { - return $user; - } - - foreach ($user as $field => $value) { - // Get the property parameter type and do the cleaning. - try { - $property = core_user::get_property_definition($field); - $user->$field = clean_param($value, $property['type']); - } catch (coding_exception $e) { - debugging("The property '$field' could not be cleaned.", DEBUG_DEVELOPER); - } - } - - return $user; + debugging('The method clean_data() has been deprecated, please use core_user::clean_data() instead.', + DEBUG_DEVELOPER); + return core_user::clean_data($user); } } diff --git a/auth/db/tests/db_test.php b/auth/db/tests/db_test.php index e0d68d3462cff..f1471cf071440 100644 --- a/auth/db/tests/db_test.php +++ b/auth/db/tests/db_test.php @@ -121,7 +121,9 @@ protected function init_auth_database() { set_config('table', $CFG->prefix.'auth_db_users', 'auth/db'); set_config('fielduser', 'name', 'auth/db'); set_config('fieldpass', 'pass', 'auth/db'); - + set_config('field_map_lastname', 'lastname', 'auth/db'); + set_config('field_updatelocal_lastname', 'oncreate', 'auth/db'); + set_config('field_lock_lastname', 'unlocked', 'auth/db'); // Setu up field mappings. set_config('field_map_email', 'email', 'auth/db'); @@ -149,7 +151,7 @@ protected function cleanup_auth_database() { public function test_plugin() { global $DB, $CFG; - $this->resetAfterTest(false); + $this->resetAfterTest(true); // NOTE: It is strongly discouraged to create new tables in advanced_testcase classes, // but there is no other simple way to test ext database enrol sync, so let's @@ -416,60 +418,31 @@ public function test_clean_data() { $extdbuser1 = (object)array('name'=>'u1', 'pass'=>'heslo', 'email'=>'u1@example.com'); $extdbuser1->id = $DB->insert_record('auth_db_users', $extdbuser1); - // User with malicious data on the name. + // User with malicious data on the name (won't be imported). $extdbuser2 = (object)array('name'=>'userxss', 'pass'=>'heslo', 'email'=>'xssuser@example.com'); $extdbuser2->id = $DB->insert_record('auth_db_users', $extdbuser2); + $extdbuser3 = (object)array('name'=>'u3', 'pass'=>'heslo', 'email'=>'u3@example.com', + 'lastname' => 'userxss'); + $extdbuser3->id = $DB->insert_record('auth_db_users', $extdbuser3); $trace = new null_progress_trace(); // Let's test user sync make sure still works as expected.. $auth->sync_users($trace, true); - - // Get the user on moodle user table. - $user2 = $DB->get_record('user', array('email'=> $extdbuser2->email, 'auth'=>'db')); - - // The malicious code should be sanitized. - $this->assertEquals($user2->username, 'userscriptalert1scriptxss'); - $this->assertNotEquals($user2->username, $extdbuser2->name); - + $this->assertDebuggingCalled("The property 'lastname' has invalid data and has been cleaned."); // User with correct data, should be equal to external db. $user1 = $DB->get_record('user', array('email'=> $extdbuser1->email, 'auth'=>'db')); $this->assertEquals($extdbuser1->name, $user1->username); $this->assertEquals($extdbuser1->email, $user1->email); - // Now, let's update the name. - $extdbuser2->name = 'user no xss anymore'; - $DB->update_record('auth_db_users', $extdbuser2); + // Get the user on moodle user table. + $user2 = $DB->get_record('user', array('email'=> $extdbuser2->email, 'auth'=>'db')); + $user3 = $DB->get_record('user', array('email'=> $extdbuser3->email, 'auth'=>'db')); - // Run sync again to update the user data. - $auth->sync_users($trace, true); + $this->assertEmpty($user2); + $this->assertEquals($extdbuser3->name, $user3->username); + $this->assertEquals('useralert(1);xss', $user3->lastname); - // The user information should be updated. - $user2 = $DB->get_record('user', array('username' => 'usernoxssanymore', 'auth' => 'db')); - // The spaces should be removed, as it's the username. - $this->assertEquals($user2->username, 'usernoxssanymore'); - - // Now let's test just the clean_data() method isolated. - // Testing PARAM_USERNAME, PARAM_NOTAGS, PARAM_RAW_TRIMMED and others. - $user3 = new stdClass(); - $user3->firstname = 'John Doe'; - $user3->username = 'john%#&~%*_doe'; - $user3->email = ' john@testing.com '; - $user3->deleted = 'no'; - $user3->description = 'A description about myself.'; - $user3cleaned = $auth->clean_data($user3); - - // Expected results. - $this->assertEquals($user3cleaned->firstname, 'John alert(1) Doe'); - $this->assertEquals($user3cleaned->email, 'john@testing.com'); - $this->assertEquals($user3cleaned->deleted, 0); - $this->assertEquals($user3->description, 'A description about myself.'); - $this->assertEquals($user3->username, 'john_doe'); - - // Try to clean an invalid property (fullname). - $user3->fullname = 'John Doe'; - $auth->clean_data($user3); - $this->assertDebuggingCalled("The property 'fullname' could not be cleaned."); $this->cleanup_auth_database(); } }