Skip to content

Commit 0833e7c

Browse files
marinaglancydanpoltawski
marinaglancy
authored and
danpoltawski
committed
MDL-58010 user: allow to update only whitelisted preferences
1 parent 10d80cd commit 0833e7c

File tree

6 files changed

+136
-7
lines changed

6 files changed

+136
-7
lines changed

‎blocks/course_overview/locallib.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ function block_course_overview_get_myorder() {
8686
// If preference was not found, look in the old location and convert if found.
8787
$order = array();
8888
if ($value = get_user_preferences('course_overview_course_order')) {
89-
$order = unserialize($value);
89+
$order = unserialize_array($value);
9090
block_course_overview_update_myorder($order);
9191
unset_user_preference('course_overview_course_order');
9292
}

‎grade/report/grader/lib.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1758,7 +1758,7 @@ protected static function get_collapsed_preferences($courseid) {
17581758

17591759
// Try looking for old location of user setting that used to store all courses in one serialized user preference.
17601760
if (($oldcollapsedpref = get_user_preferences('grade_report_grader_collapsed_categories')) !== null) {
1761-
if ($collapsedall = @unserialize($oldcollapsedpref)) {
1761+
if ($collapsedall = unserialize_array($oldcollapsedpref)) {
17621762
// We found the old-style preference, filter out only categories that belong to this course and update the prefs.
17631763
$collapsed = static::filter_collapsed_categories($courseid, $collapsedall);
17641764
if (!empty($collapsed['aggregatesonly']) || !empty($collapsed['gradesonly'])) {

‎lib/moodlelib.php‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9512,6 +9512,58 @@ function get_course_display_name_for_list($course) {
95129512
}
95139513
}
95149514

9515+
/**
9516+
* Safe analogue of unserialize() that can only parse arrays
9517+
*
9518+
* Arrays may contain only integers or strings as both keys and values. Nested arrays are allowed.
9519+
* Note: If any string (key or value) has semicolon (;) as part of the string parsing will fail.
9520+
* This is a simple method to substitute unnecessary unserialize() in code and not intended to cover all possible cases.
9521+
*
9522+
* @param string $expression
9523+
* @return array|bool either parsed array or false if parsing was impossible.
9524+
*/
9525+
function unserialize_array($expression) {
9526+
$subs = [];
9527+
// Find nested arrays, parse them and store in $subs , substitute with special string.
9528+
while (preg_match('/([\^;\}])(a:\d+:\{[^\{\}]*\})/', $expression, $matches) && strlen($matches[2]) < strlen($expression)) {
9529+
$key = '--SUB' . count($subs) . '--';
9530+
$subs[$key] = unserialize_array($matches[2]);
9531+
if ($subs[$key] === false) {
9532+
return false;
9533+
}
9534+
$expression = str_replace($matches[2], $key . ';', $expression);
9535+
}
9536+
9537+
// Check the expression is an array.
9538+
if (!preg_match('/^a:(\d+):\{([^\}]*)\}$/', $expression, $matches1)) {
9539+
return false;
9540+
}
9541+
// Get the size and elements of an array (key;value;key;value;....).
9542+
$parts = explode(';', $matches1[2]);
9543+
$size = intval($matches1[1]);
9544+
if (count($parts) < $size * 2 + 1) {
9545+
return false;
9546+
}
9547+
// Analyze each part and make sure it is an integer or string or a substitute.
9548+
$value = [];
9549+
for ($i = 0; $i < $size * 2; $i++) {
9550+
if (preg_match('/^i:(\d+)$/', $parts[$i], $matches2)) {
9551+
$parts[$i] = (int)$matches2[1];
9552+
} else if (preg_match('/^s:(\d+):"(.*)"$/', $parts[$i], $matches3) && strlen($matches3[2]) == (int)$matches3[1]) {
9553+
$parts[$i] = $matches3[2];
9554+
} else if (preg_match('/^--SUB\d+--$/', $parts[$i])) {
9555+
$parts[$i] = $subs[$parts[$i]];
9556+
} else {
9557+
return false;
9558+
}
9559+
}
9560+
// Combine keys and values.
9561+
for ($i = 0; $i < $size * 2; $i += 2) {
9562+
$value[$parts[$i]] = $parts[$i+1];
9563+
}
9564+
return $value;
9565+
}
9566+
95159567
/**
95169568
* The lang_string class
95179569
*

‎lib/tests/moodlelib_test.php‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3036,4 +3036,34 @@ public function test_generate_email_processing_address() {
30363036
$CFG->mailprefix = 'mdl-';
30373037
$this->assertEquals(1, validate_email(generate_email_processing_address(23, $modargs)));
30383038
}
3039+
3040+
/**
3041+
* Test safe method unserialize_array().
3042+
*/
3043+
public function test_unserialize_array() {
3044+
$a = [1, 2, 3];
3045+
$this->assertEquals($a, unserialize_array(serialize($a)));
3046+
$this->assertEquals($a, unserialize_array(serialize($a)));
3047+
$a = ['a' => 1, 2 => 2, 'b' => 'cde'];
3048+
$this->assertEquals($a, unserialize_array(serialize($a)));
3049+
$this->assertEquals($a, unserialize_array(serialize($a)));
3050+
$a = ['a' => 1, 2 => 2, 'b' => 'c"d"e'];
3051+
$this->assertEquals($a, unserialize_array(serialize($a)));
3052+
$a = ['a' => 1, 2 => ['c' => 'd', 'e' => 'f'], 'b' => 'cde'];
3053+
$this->assertEquals($a, unserialize_array(serialize($a)));
3054+
3055+
// Can not unserialize if any string contains semicolons.
3056+
$a = ['a' => 1, 2 => 2, 'b' => 'c"d";e'];
3057+
$this->assertEquals(false, unserialize_array(serialize($a)));
3058+
3059+
// Can not unserialize if there are any objects.
3060+
$a = (object)['a' => 1, 2 => 2, 'b' => 'cde'];
3061+
$this->assertEquals(false, unserialize_array(serialize($a)));
3062+
$a = ['a' => 1, 2 => 2, 'b' => (object)['a' => 'cde']];
3063+
$this->assertEquals(false, unserialize_array(serialize($a)));
3064+
3065+
// Array used in the grader report.
3066+
$a = array('aggregatesonly' => [51, 34], 'gradesonly' => [21, 45, 78]);
3067+
$this->assertEquals($a, unserialize_array(serialize($a)));
3068+
}
30393069
}

‎user/externallib.php‎

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -217,8 +217,8 @@ public static function create_users($users) {
217217
profile_save_data((object) $user);
218218
}
219219

220+
$userobject = (object)$user;
220221
if ($createpassword) {
221-
$userobject = (object)$user;
222222
setnew_password_and_mail($userobject);
223223
unset_user_preference('create_password', $userobject);
224224
set_user_preference('auth_forcepasswordchange', 1, $userobject);
@@ -230,7 +230,7 @@ public static function create_users($users) {
230230
// Preferences.
231231
if (!empty($user['preferences'])) {
232232
foreach ($user['preferences'] as $preference) {
233-
set_user_preference($preference['type'], $preference['value'], $user['id']);
233+
self::set_user_preference($preference['type'], $preference['value'], $userobject);
234234
}
235235
}
236236

@@ -459,7 +459,7 @@ public static function update_users($users) {
459459
// Preferences.
460460
if (!empty($user['preferences'])) {
461461
foreach ($user['preferences'] as $preference) {
462-
set_user_preference($preference['type'], $preference['value'], $user['id']);
462+
self::set_user_preference($preference['type'], $preference['value'], $existinguser);
463463
}
464464
}
465465
}
@@ -1453,6 +1453,35 @@ public static function view_user_profile_returns() {
14531453
);
14541454
}
14551455

1456+
/**
1457+
* Validates preference value and updates the user preference
1458+
*
1459+
* @param string $name
1460+
* @param string $value
1461+
* @param stdClass $user
1462+
*/
1463+
protected static function set_user_preference($name, $value, $user) {
1464+
$preferences = array(
1465+
'auth_forcepasswordchange' => PARAM_BOOL,
1466+
'htmleditor' => PARAM_COMPONENT,
1467+
'usemodchooser' => PARAM_BOOL,
1468+
'badgeprivacysetting' => PARAM_BOOL,
1469+
'blogpagesize' => PARAM_INT,
1470+
'forum_markasreadonnotification' => PARAM_INT,
1471+
'calendar_timeformat' => PARAM_NOTAGS,
1472+
'calendar_startwday' => PARAM_INT,
1473+
'calendar_maxevents' => PARAM_INT,
1474+
'calendar_lookahead' => PARAM_INT,
1475+
'calendar_persistflt' => PARAM_INT
1476+
);
1477+
if (isset($preferences[$name])) {
1478+
$value = clean_param($value, $preferences[$name]);
1479+
if ($preferences[$name] == PARAM_BOOL) {
1480+
$value = (int)$value;
1481+
}
1482+
set_user_preference($name, $value, $user);
1483+
}
1484+
}
14561485
}
14571486

14581487
/**

‎user/tests/externallib_test.php‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -497,7 +497,14 @@ public function test_create_users() {
497497
'email' => 'usertest1@example.com',
498498
'description' => 'This is a description for user 1',
499499
'city' => 'Perth',
500-
'country' => 'au'
500+
'country' => 'au',
501+
'preferences' => [[
502+
'type' => 'htmleditor',
503+
'value' => 'atto'
504+
], [
505+
'type' => 'invalidpreference',
506+
'value' => 'abcd'
507+
]]
501508
);
502509

503510
$context = context_system::instance();
@@ -522,6 +529,8 @@ public function test_create_users() {
522529
$this->assertEquals($dbuser->description, $user1['description']);
523530
$this->assertEquals($dbuser->city, $user1['city']);
524531
$this->assertEquals($dbuser->country, $user1['country']);
532+
$this->assertEquals('atto', get_user_preferences('htmleditor', null, $dbuser));
533+
$this->assertEquals(null, get_user_preferences('invalidpreference', null, $dbuser));
525534
}
526535

527536
// Call without required capability
@@ -676,7 +685,14 @@ public function test_update_users() {
676685
'email' => 'usertest1@example.com',
677686
'description' => 'This is a description for user 1',
678687
'city' => 'Perth',
679-
'country' => 'au'
688+
'country' => 'au',
689+
'preferences' => [[
690+
'type' => 'htmleditor',
691+
'value' => 'atto'
692+
], [
693+
'type' => 'invalidpreference',
694+
'value' => 'abcd'
695+
]]
680696
);
681697

682698
$context = context_system::instance();
@@ -712,6 +728,8 @@ public function test_update_users() {
712728
$this->assertEquals($dbuser->description, $user1['description']);
713729
$this->assertEquals($dbuser->city, $user1['city']);
714730
$this->assertEquals($dbuser->country, $user1['country']);
731+
$this->assertEquals('atto', get_user_preferences('htmleditor', null, $dbuser));
732+
$this->assertEquals(null, get_user_preferences('invalidpreference', null, $dbuser));
715733

716734
// Call without required capability.
717735
$this->unassignUserCapability('moodle/user:update', $context->id, $roleid);

0 commit comments

Comments
 (0)