Skip to content

Commit

Permalink
MDL-36721 do not store passwords in config logs
Browse files Browse the repository at this point in the history
  • Loading branch information
@skodak
skodak committed Dec 27, 2013
1 parent bbb291b commit 914499a
Show file tree
Hide file tree
Showing 4 changed files with 167 additions and 2 deletions.
28 changes: 27 additions & 1 deletion lib/adminlib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1660,11 +1660,21 @@ public function config_write($name, $value) {
rebuild_course_cache(0, true);
}

add_to_config_log($name, $oldvalue, $value, $this->plugin);
$this->add_to_config_log($name, $oldvalue, $value);

return true; // BC only
}

/**
* Log config changes if necessary.
* @param string $name
* @param string $oldvalue
* @param string $value
*/
protected function add_to_config_log($name, $oldvalue, $value) {
add_to_config_log($name, $oldvalue, $value, $this->plugin);
}

/**
* Returns current value of this setting
* @return mixed array or string depending on instance, NULL means not set yet
Expand Down Expand Up @@ -2161,6 +2171,22 @@ public function __construct($name, $visiblename, $description, $defaultsetting)
parent::__construct($name, $visiblename, $description, $defaultsetting, PARAM_RAW, 30);
}

/**
* Log config changes if necessary.
* @param string $name
* @param string $oldvalue
* @param string $value
*/
protected function add_to_config_log($name, $oldvalue, $value) {
if ($value !== '') {
$value = '********';
}
if ($oldvalue !== '' and $oldvalue !== null) {
$oldvalue = '********';
}
parent::add_to_config_log($name, $oldvalue, $value);
}

/**
* Returns XHTML for the field
* Writes Javascript into the HTML below right before the last div
Expand Down
39 changes: 39 additions & 0 deletions lib/db/upgrade.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2862,5 +2862,44 @@ function xmldb_main_upgrade($oldversion) {
upgrade_main_savepoint(true, 2013111800.01);
}

if ($oldversion < 2013122400.01) {
// Purge stored passwords from config_log table, ideally this should be in each plugin
// but that would complicate backporting...
$items = array(
'core/cronremotepassword', 'core/proxypassword', 'core/smtppass', 'core/jabberpassword',
'enrol_database/dbpass', 'enrol_ldap/bind_pw', 'url/secretphrase');
foreach ($items as $item) {
list($plugin, $name) = explode('/', $item);
if ($plugin === 'core') {
$sql = "UPDATE {config_log}
SET value = :value
WHERE name = :name AND plugin IS NULL AND value <> ''";
$params = array('value'=>'********', 'name'=>$name);
$DB->execute($sql, $params);

$sql = "UPDATE {config_log}
SET oldvalue = :value
WHERE name = :name AND plugin IS NULL AND oldvalue <> ''";
$params = array('value'=>'********', 'name'=>$name);
$DB->execute($sql, $params);

} else {
$sql = "UPDATE {config_log}
SET value = :value
WHERE name = :name AND plugin = :plugin AND value <> ''";
$params = array('value'=>'********', 'name'=>$name, 'plugin'=>$plugin);
$DB->execute($sql, $params);

$sql = "UPDATE {config_log}
SET oldvalue = :value
WHERE name = :name AND plugin = :plugin AND oldvalue <> ''";
$params = array('value'=>'********', 'name'=>$name, 'plugin'=>$plugin);
$DB->execute($sql, $params);
}
}
// Main savepoint reached.
upgrade_main_savepoint(true, 2013122400.01);
}

return true;
}
100 changes: 100 additions & 0 deletions lib/tests/admintree_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,104 @@ public function test_add_nodes_before_invalid2() {
$tree = new admin_root(true);
$tree->add('root', new admin_category('bar', 'Bar'), '');
}

/**
* Saving of values.
*/
public function test_config_logging() {
global $DB;
$this->resetAfterTest();

$DB->delete_records('config_log', array());

$adminroot = new admin_root(true);
$adminroot->add('root', $one = new admin_category('one', 'One'));
$page = new admin_settingpage('page', 'Page');
$page->add(new admin_setting_configtext('text1', 'Text 1', '', ''));
$page->add(new admin_setting_configpasswordunmask('pass1', 'Password 1', '', ''));
$adminroot->add('one', $page);

$this->assertEmpty($DB->get_records('config_log'));
$data = array('s__text1'=>'sometext', 's__pass1'=>'');
$count = $this->save_config_data($adminroot, $data);

$this->assertEquals(2, $count);
$records = $DB->get_records('config_log', array(), 'id asc');
$this->assertCount(2, $records);
reset($records);
$record = array_shift($records);
$this->assertNull($record->plugin);
$this->assertSame('text1', $record->name);
$this->assertNull($record->oldvalue);
$this->assertSame('sometext', $record->value);
$record = array_shift($records);
$this->assertNull($record->plugin);
$this->assertSame('pass1', $record->name);
$this->assertNull($record->oldvalue);
$this->assertSame('', $record->value);

$DB->delete_records('config_log', array());
$data = array('s__text1'=>'other', 's__pass1'=>'nice password');
$count = $this->save_config_data($adminroot, $data);

$this->assertEquals(2, $count);
$records = $DB->get_records('config_log', array(), 'id asc');
$this->assertCount(2, $records);
reset($records);
$record = array_shift($records);
$this->assertNull($record->plugin);
$this->assertSame('text1', $record->name);
$this->assertSame('sometext', $record->oldvalue);
$this->assertSame('other', $record->value);
$record = array_shift($records);
$this->assertNull($record->plugin);
$this->assertSame('pass1', $record->name);
$this->assertSame('', $record->oldvalue);
$this->assertSame('********', $record->value);

$DB->delete_records('config_log', array());
$data = array('s__text1'=>'', 's__pass1'=>'');
$count = $this->save_config_data($adminroot, $data);

$this->assertEquals(2, $count);
$records = $DB->get_records('config_log', array(), 'id asc');
$this->assertCount(2, $records);
reset($records);
$record = array_shift($records);
$this->assertNull($record->plugin);
$this->assertSame('text1', $record->name);
$this->assertSame('other', $record->oldvalue);
$this->assertSame('', $record->value);
$record = array_shift($records);
$this->assertNull($record->plugin);
$this->assertSame('pass1', $record->name);
$this->assertSame('********', $record->oldvalue);
$this->assertSame('', $record->value);
}

protected function save_config_data(admin_root $adminroot, array $data) {
$adminroot->errors = array();

$settings = admin_find_write_settings($adminroot, $data);

$count = 0;
foreach ($settings as $fullname=>$setting) {
/** @var $setting admin_setting */
$original = $setting->get_setting();
$error = $setting->write_setting($data[$fullname]);
if ($error !== '') {
$adminroot->errors[$fullname] = new stdClass();
$adminroot->errors[$fullname]->data = $data[$fullname];
$adminroot->errors[$fullname]->id = $setting->get_id();
$adminroot->errors[$fullname]->error = $error;
} else {
$setting->write_setting_flags($data);
}
if ($setting->post_write_settings($original)) {
$count++;
}
}

return $count;
}
}
2 changes: 1 addition & 1 deletion version.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@

defined('MOODLE_INTERNAL') || die();

$version = 2013122400.00; // YYYYMMDD = weekly release date of this DEV branch.
$version = 2013122400.01; // YYYYMMDD = weekly release date of this DEV branch.
// RR = release increments - 00 in DEV branches.
// .XX = incremental changes.

Expand Down

0 comments on commit 914499a

Please sign in to comment.