Skip to content

Commit

Permalink
MDL-52781 auth_db: deprecate clean_data method.
Browse files Browse the repository at this point in the history 
    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.
  • Loading branch information
@lameze
lameze committed Apr 21, 2016
1 parent ac9768f commit 5e60be8
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 62 deletions.
24 changes: 4 additions & 20 deletions auth/db/auth.php
View file
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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);
}
}

Expand Down
57 changes: 15 additions & 42 deletions auth/db/tests/db_test.php
View file
Expand Up @@ -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');
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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'=>'user<script>alert(1);</script>xss', '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' => 'user<script>alert(1);</script>xss');
$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 <script>alert(1)</script> Doe';
$user3->username = 'john%#&~%*_doe';
$user3->email = ' john@testing.com ';
$user3->deleted = 'no';
$user3->description = '<b>A description <script>alert(123)</script>about myself.</b>';
$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, '<b>A description about myself.</b>');
$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();
}
}

0 comments on commit 5e60be8

Please sign in to comment.