diff --git a/auth/db/auth.php b/auth/db/auth.php index e35b94df5fc..5cf9efdaa6d 100644 --- a/auth/db/auth.php +++ b/auth/db/auth.php @@ -300,6 +300,7 @@ class auth_plugin_db extends auth_plugin_base { $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); } @@ -381,6 +382,7 @@ class auth_plugin_db extends auth_plugin_base { foreach($add_users as $user) { $username = $user; if ($this->config->removeuser == AUTH_REMOVEUSER_SUSPEND) { + if ($old_user = $DB->get_record('user', array('username'=>$username, 'deleted'=>0, 'suspended'=>1, 'mnethostid'=>$CFG->mnet_localhost_id, 'auth'=>$this->authtype))) { $DB->set_field('user', 'suspended', 0, array('id'=>$old_user->id)); $trace->output(get_string('auth_dbreviveduser', 'auth_db', array('name'=>$username, 'id'=>$old_user->id)), 1); @@ -412,6 +414,7 @@ class auth_plugin_db extends auth_plugin_base { $trace->output(get_string('auth_dbinsertuserduplicate', 'auth_db', array('username'=>$user->username, 'auth'=>$collision->auth)), 1); continue; } + $user = $this->clean_data($user); try { $id = $DB->insert_record ('user', $user); // it is truly a new user @@ -553,6 +556,7 @@ class auth_plugin_db extends auth_plugin_base { } } } + if ($updated) { $DB->set_field('user', 'timemodified', time(), array('id'=>$userid)); @@ -870,6 +874,30 @@ class auth_plugin_db extends auth_plugin_base { error_reporting($CFG->debug); ob_end_flush(); } + + /** + * Clean the user data that comes from an external database. + * + * @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; + } } diff --git a/auth/db/tests/db_test.php b/auth/db/tests/db_test.php index 356bf467f50..5b289bb1925 100644 --- a/auth/db/tests/db_test.php +++ b/auth/db/tests/db_test.php @@ -374,4 +374,77 @@ class auth_db_testcase extends advanced_testcase { $this->cleanup_auth_database(); } + + /** + * Testing the clean_data() method. + */ + public function test_clean_data() { + global $DB; + + $this->resetAfterTest(false); + $this->preventResetByRollback(); + $this->init_auth_database(); + $auth = get_auth_plugin('db'); + $auth->db_init(); + + // Create users on external table. + $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. + $extdbuser2 = (object)array('name'=>'userxss', 'pass'=>'heslo', 'email'=>'xssuser@example.com'); + $extdbuser2->id = $DB->insert_record('auth_db_users', $extdbuser2); + + $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); + + // 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); + + // Run sync again to update the user data. + $auth->sync_users($trace, true); + + // 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(); + } }