From 6e65554ea19f4e90c09864081e47424f8efca02e Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Sun, 5 Mar 2017 13:20:03 +0800 Subject: [PATCH] MDL-58010 user: allow to update only whitelisted user preferences --- badges/preferences.php | 5 +- blocks/course_overview/locallib.php | 2 +- blog/preferences.php | 4 +- calendar/lib.php | 22 ++ grade/report/grader/lib.php | 2 +- lib/classes/user.php | 220 ++++++++++++++++++ lib/moodlelib.php | 54 +++++ lib/tests/moodlelib_test.php | 30 +++ message/externallib.php | 7 +- message/lib.php | 66 ++++++ message/output/email/message_output_email.php | 5 +- user/calendar.php | 32 +-- user/course.php | 3 +- user/edit.php | 16 +- user/editlib.php | 28 ++- user/editor.php | 2 +- user/externallib.php | 69 +++--- user/tests/externallib_test.php | 87 +++++-- 18 files changed, 559 insertions(+), 95 deletions(-) diff --git a/badges/preferences.php b/badges/preferences.php index 5152089925d..7e01e2c5f2a 100644 --- a/badges/preferences.php +++ b/badges/preferences.php @@ -26,6 +26,7 @@ require_once(__DIR__ . '/../config.php'); require_once('preferences_form.php'); +require_once($CFG->dirroot.'/user/editlib.php'); $url = new moodle_url('/badges/preferences.php'); @@ -42,8 +43,8 @@ $mform = new badges_preferences_form(); $mform->set_data(array('badgeprivacysetting' => get_user_preferences('badgeprivacysetting'))); if (!$mform->is_cancelled() && $data = $mform->get_data()) { - $setting = $data->badgeprivacysetting; - set_user_preference('badgeprivacysetting', $setting); + useredit_update_user_preference(['id' => $USER->id, + 'preference_badgeprivacysetting' => $data->badgeprivacysetting]); } if ($mform->is_cancelled()) { diff --git a/blocks/course_overview/locallib.php b/blocks/course_overview/locallib.php index 746ccf4eee1..06e6896ce3d 100644 --- a/blocks/course_overview/locallib.php +++ b/blocks/course_overview/locallib.php @@ -86,7 +86,7 @@ function block_course_overview_get_myorder() { // If preference was not found, look in the old location and convert if found. $order = array(); if ($value = get_user_preferences('course_overview_course_order')) { - $order = unserialize($value); + $order = unserialize_array($value); block_course_overview_update_myorder($order); unset_user_preference('course_overview_course_order'); } diff --git a/blog/preferences.php b/blog/preferences.php index 18711612945..cd89980dd5d 100644 --- a/blog/preferences.php +++ b/blog/preferences.php @@ -27,6 +27,7 @@ require_once('../config.php'); require_once($CFG->dirroot.'/blog/lib.php'); require_once('preferences_form.php'); +require_once($CFG->dirroot.'/user/editlib.php'); $courseid = optional_param('courseid', SITEID, PARAM_INT); $modid = optional_param('modid', null, PARAM_INT); @@ -81,7 +82,8 @@ if (!$mform->is_cancelled() && $data = $mform->get_data()) { if ($pagesize < 1) { print_error('invalidpagesize'); } - set_user_preference('blogpagesize', $pagesize); + useredit_update_user_preference(['id' => $USER->id, + 'preference_blogpagesize' => $pagesize]); } if ($mform->is_cancelled()) { diff --git a/calendar/lib.php b/calendar/lib.php index fd879fa3189..0ef3d9ef1b2 100644 --- a/calendar/lib.php +++ b/calendar/lib.php @@ -3449,3 +3449,25 @@ function calendar_get_calendar_context($subscription) { } return $context; } + +/** + * Implements callback user_preferences, whitelists preferences that users are allowed to update directly + * + * Used in {@see core_user::fill_preferences_cache()}, see also {@see useredit_update_user_preference()} + * + * @return array + */ +function core_calendar_user_preferences() { + $preferences = []; + $preferences['calendar_timeformat'] = array('type' => PARAM_NOTAGS, 'null' => NULL_NOT_ALLOWED, 'default' => '0', + 'choices' => array('0', CALENDAR_TF_12, CALENDAR_TF_24) + ); + $preferences['calendar_startwday'] = array('type' => PARAM_INT, 'null' => NULL_NOT_ALLOWED, 'default' => 0, + 'choices' => array(0, 1, 2, 3, 4, 5, 6)); + $preferences['calendar_maxevents'] = array('type' => PARAM_INT, 'choices' => range(1, 20)); + $preferences['calendar_lookahead'] = array('type' => PARAM_INT, 'null' => NULL_NOT_ALLOWED, 'default' => 365, + 'choices' => array(365, 270, 180, 150, 120, 90, 60, 30, 21, 14, 7, 6, 5, 4, 3, 2, 1)); + $preferences['calendar_persistflt'] = array('type' => PARAM_INT, 'null' => NULL_NOT_ALLOWED, 'default' => 0, + 'choices' => array(0, 1)); + return $preferences; +} \ No newline at end of file diff --git a/grade/report/grader/lib.php b/grade/report/grader/lib.php index ae3e29bcbbd..8d7eb1391ce 100644 --- a/grade/report/grader/lib.php +++ b/grade/report/grader/lib.php @@ -1774,7 +1774,7 @@ class grade_report_grader extends grade_report { // Try looking for old location of user setting that used to store all courses in one serialized user preference. if (($oldcollapsedpref = get_user_preferences('grade_report_grader_collapsed_categories')) !== null) { - if ($collapsedall = @unserialize($oldcollapsedpref)) { + if ($collapsedall = unserialize_array($oldcollapsedpref)) { // We found the old-style preference, filter out only categories that belong to this course and update the prefs. $collapsed = static::filter_collapsed_categories($courseid, $collapsedall); if (!empty($collapsed['aggregatesonly']) || !empty($collapsed['gradesonly'])) { diff --git a/lib/classes/user.php b/lib/classes/user.php index 88b1494f706..8ae89dcc87e 100644 --- a/lib/classes/user.php +++ b/lib/classes/user.php @@ -67,6 +67,9 @@ class core_user { /** @var array store user fields properties cache. */ protected static $propertiescache = null; + /** @var array store user preferences cache. */ + protected static $preferencescache = null; + /** * Return user object from db or create noreply or support user, * if userid matches corse_user::NOREPLY_USER or corse_user::SUPPORT_USER @@ -644,4 +647,221 @@ class core_user { return self::$propertiescache[$property]['default']; } + + /** + * Definition of updateable user preferences and rules for data and access validation. + * + * array( + * 'preferencename' => array( // Either exact preference name or a regular expression. + * 'null' => NULL_ALLOWED, // Defaults to NULL_NOT_ALLOWED. Takes NULL_NOT_ALLOWED or NULL_ALLOWED. + * 'type' => PARAM_TYPE, // Expected parameter type of the user field - mandatory + * 'choices' => array(1, 2..) // An array of accepted values of the user field - optional + * 'default' => $CFG->setting // An default value for the field - optional + * 'isregex' => false/true // Whether the name of the preference is a regular expression (default false). + * 'permissioncallback' => callable // Function accepting arguments ($user, $preferencename) that checks if current user + * // is allowed to modify this preference for given user. + * // If not specified core_user::default_preference_permission_check() will be assumed. + * 'cleancallback' => callable // Custom callback for cleaning value if something more difficult than just type/choices is needed + * // accepts arguments ($value, $preferencename) + * ) + * ) + * + * @return void + */ + protected static function fill_preferences_cache() { + if (self::$preferencescache !== null) { + return; + } + + // Array of user preferences and expected types/values. + // Every preference that can be updated directly by user should be added here. + $preferences = array(); + $preferences['auth_forcepasswordchange'] = array('type' => PARAM_INT, 'null' => NULL_NOT_ALLOWED, 'choices' => array(0, 1), + 'permissioncallback' => function($user, $preferencename) { + global $USER; + $systemcontext = context_system::instance(); + return ($USER->id != $user->id && (has_capability('moodle/user:update', $systemcontext) || + ($user->timecreated > time() - 10 && has_capability('moodle/user:create', $systemcontext)))); + }); + $preferences['usemodchooser'] = array('type' => PARAM_INT, 'null' => NULL_NOT_ALLOWED, 'default' => 1, + 'choices' => array(0, 1)); + $preferences['forum_markasreadonnotification'] = array('type' => PARAM_INT, 'null' => NULL_NOT_ALLOWED, 'default' => 1, + 'choices' => array(0, 1)); + $preferences['htmleditor'] = array('type' => PARAM_NOTAGS, 'null' => NULL_ALLOWED, + 'cleancallback' => function($value, $preferencename) { + if (empty($value) || !array_key_exists($value, core_component::get_plugin_list('editor'))) { + return null; + } + return $value; + }); + $preferences['badgeprivacysetting'] = array('type' => PARAM_INT, 'null' => NULL_NOT_ALLOWED, 'default' => 1, + 'choices' => array(0, 1), 'permissioncallback' => function($user, $preferencename) { + global $CFG, $USER; + return !empty($CFG->enablebadges) && $user->id == $USER->id; + }); + $preferences['blogpagesize'] = array('type' => PARAM_INT, 'null' => NULL_NOT_ALLOWED, 'default' => 10, + 'permissioncallback' => function($user, $preferencename) { + global $USER; + return $USER->id == $user->id && has_capability('moodle/blog:view', context_system::instance()); + }); + + // Core components that may want to define their preferences. + // List of core components implementing callback is hardcoded here for performance reasons. + // TODO MDL-58184 cache list of core components implementing a function. + $corecomponents = ['core_message', 'core_calendar']; + foreach ($corecomponents as $component) { + if (($pluginpreferences = component_callback($component, 'user_preferences')) && is_array($pluginpreferences)) { + $preferences += $pluginpreferences; + } + } + + // Plugins that may define their preferences. + if ($pluginsfunction = get_plugins_with_function('user_preferences')) { + foreach ($pluginsfunction as $plugintype => $plugins) { + foreach ($plugins as $function) { + if (($pluginpreferences = call_user_func($function)) && is_array($pluginpreferences)) { + $preferences += $pluginpreferences; + } + } + } + } + + self::$preferencescache = $preferences; + } + + /** + * Retrieves the preference definition + * + * @param string $preferencename + * @return array + */ + protected static function get_preference_definition($preferencename) { + self::fill_preferences_cache(); + + foreach (self::$preferencescache as $key => $preference) { + if (empty($preference['isregex'])) { + if ($key === $preferencename) { + return $preference; + } + } else { + if (preg_match($key, $preferencename)) { + return $preference; + } + } + } + + throw new coding_exception('Invalid preference requested.'); + } + + /** + * Default callback used for checking if current user is allowed to change permission of user $user + * + * @param stdClass $user + * @param string $preferencename + * @return bool + */ + protected static function default_preference_permission_check($user, $preferencename) { + global $USER; + if (is_mnet_remote_user($user)) { + // Can't edit MNET user. + return false; + } + + if ($user->id == $USER->id) { + // Editing own profile. + $systemcontext = context_system::instance(); + return has_capability('moodle/user:editownprofile', $systemcontext); + } else { + // Teachers, parents, etc. + $personalcontext = context_user::instance($user->id); + if (!has_capability('moodle/user:editprofile', $personalcontext)) { + return false; + } + if (is_siteadmin($user->id) and !is_siteadmin($USER)) { + // Only admins may edit other admins. + return false; + } + return true; + } + } + + /** + * Can current user edit preference of this/another user + * + * @param string $preferencename + * @param stdClass $user + * @return bool + */ + public static function can_edit_preference($preferencename, $user) { + if (!isloggedin() || isguestuser()) { + // Guests can not edit anything. + return false; + } + + try { + $definition = self::get_preference_definition($preferencename); + } catch (coding_exception $e) { + return false; + } + + if ($user->deleted || !context_user::instance($user->id, IGNORE_MISSING)) { + // User is deleted. + return false; + } + + if (isset($definition['permissioncallback'])) { + $callback = $definition['permissioncallback']; + if (is_callable($callback)) { + return call_user_func_array($callback, [$user, $preferencename]); + } else { + throw new coding_exception('Permission callback for preference ' . s($preferencename) . ' is not callable'); + return false; + } + } else { + return self::default_preference_permission_check($user, $preferencename); + } + } + + /** + * Clean value of a user preference + * + * @param string $value the user preference value to be cleaned. + * @param string $preferencename the user preference name + * @return string the cleaned preference value + */ + public static function clean_preference($value, $preferencename) { + + $definition = self::get_preference_definition($preferencename); + + if (isset($definition['type']) && $value !== null) { + $value = clean_param($value, $definition['type']); + } + + if (isset($definition['cleancallback'])) { + $callback = $definition['cleancallback']; + if (is_callable($callback)) { + return $callback($value, $preferencename); + } else { + throw new coding_exception('Clean callback for preference ' . s($preferencename) . ' is not callable'); + } + } else if ($value === null && (!isset($definition['null']) || $definition['null'] == NULL_ALLOWED)) { + return null; + } else if (isset($definition['choices'])) { + if (!in_array($value, $definition['choices'])) { + if (isset($definition['default'])) { + return $definition['default']; + } else { + $first = reset($definition['choices']); + return $first; + } + } else { + return $value; + } + } else { + if ($value === null) { + return isset($definition['default']) ? $definition['default'] : ''; + } + return $value; + } + } } diff --git a/lib/moodlelib.php b/lib/moodlelib.php index b939f2a0445..22f6f5bd647 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -1864,6 +1864,8 @@ function mark_user_preferences_changed($userid) { * * If a $user object is submitted it's 'preference' property is used for the preferences cache. * + * When additional validation/permission check is needed it is better to use {@see useredit_update_user_preference()} + * * @package core * @category preference * @access public @@ -9702,6 +9704,58 @@ function get_course_display_name_for_list($course) { } } +/** + * Safe analogue of unserialize() that can only parse arrays + * + * Arrays may contain only integers or strings as both keys and values. Nested arrays are allowed. + * Note: If any string (key or value) has semicolon (;) as part of the string parsing will fail. + * This is a simple method to substitute unnecessary unserialize() in code and not intended to cover all possible cases. + * + * @param string $expression + * @return array|bool either parsed array or false if parsing was impossible. + */ +function unserialize_array($expression) { + $subs = []; + // Find nested arrays, parse them and store in $subs , substitute with special string. + while (preg_match('/([\^;\}])(a:\d+:\{[^\{\}]*\})/', $expression, $matches) && strlen($matches[2]) < strlen($expression)) { + $key = '--SUB' . count($subs) . '--'; + $subs[$key] = unserialize_array($matches[2]); + if ($subs[$key] === false) { + return false; + } + $expression = str_replace($matches[2], $key . ';', $expression); + } + + // Check the expression is an array. + if (!preg_match('/^a:(\d+):\{([^\}]*)\}$/', $expression, $matches1)) { + return false; + } + // Get the size and elements of an array (key;value;key;value;....). + $parts = explode(';', $matches1[2]); + $size = intval($matches1[1]); + if (count($parts) < $size * 2 + 1) { + return false; + } + // Analyze each part and make sure it is an integer or string or a substitute. + $value = []; + for ($i = 0; $i < $size * 2; $i++) { + if (preg_match('/^i:(\d+)$/', $parts[$i], $matches2)) { + $parts[$i] = (int)$matches2[1]; + } else if (preg_match('/^s:(\d+):"(.*)"$/', $parts[$i], $matches3) && strlen($matches3[2]) == (int)$matches3[1]) { + $parts[$i] = $matches3[2]; + } else if (preg_match('/^--SUB\d+--$/', $parts[$i])) { + $parts[$i] = $subs[$parts[$i]]; + } else { + return false; + } + } + // Combine keys and values. + for ($i = 0; $i < $size * 2; $i += 2) { + $value[$parts[$i]] = $parts[$i+1]; + } + return $value; +} + /** * The lang_string class * diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php index 4b48a49bc0d..c73a5b352a3 100644 --- a/lib/tests/moodlelib_test.php +++ b/lib/tests/moodlelib_test.php @@ -3512,4 +3512,34 @@ class core_moodlelib_testcase extends advanced_testcase { $CFG->mailprefix = 'mdl-'; $this->assertTrue(validate_email(generate_email_processing_address(23, $modargs))); } + + /** + * Test safe method unserialize_array(). + */ + public function test_unserialize_array() { + $a = [1, 2, 3]; + $this->assertEquals($a, unserialize_array(serialize($a))); + $this->assertEquals($a, unserialize_array(serialize($a))); + $a = ['a' => 1, 2 => 2, 'b' => 'cde']; + $this->assertEquals($a, unserialize_array(serialize($a))); + $this->assertEquals($a, unserialize_array(serialize($a))); + $a = ['a' => 1, 2 => 2, 'b' => 'c"d"e']; + $this->assertEquals($a, unserialize_array(serialize($a))); + $a = ['a' => 1, 2 => ['c' => 'd', 'e' => 'f'], 'b' => 'cde']; + $this->assertEquals($a, unserialize_array(serialize($a))); + + // Can not unserialize if any string contains semicolons. + $a = ['a' => 1, 2 => 2, 'b' => 'c"d";e']; + $this->assertEquals(false, unserialize_array(serialize($a))); + + // Can not unserialize if there are any objects. + $a = (object)['a' => 1, 2 => 2, 'b' => 'cde']; + $this->assertEquals(false, unserialize_array(serialize($a))); + $a = ['a' => 1, 2 => 2, 'b' => (object)['a' => 'cde']]; + $this->assertEquals(false, unserialize_array(serialize($a))); + + // Array used in the grader report. + $a = array('aggregatesonly' => [51, 34], 'gradesonly' => [21, 45, 78]); + $this->assertEquals($a, unserialize_array(serialize($a))); + } } diff --git a/message/externallib.php b/message/externallib.php index bb5ec863649..b0f38eb1217 100644 --- a/message/externallib.php +++ b/message/externallib.php @@ -2201,12 +2201,7 @@ class core_message_external extends external_api { ) ); - if (empty($params['userid'])) { - $params['userid'] = $USER->id; - } - - $user = core_user::get_user($params['userid'], '*', MUST_EXIST); - core_user::require_active_user($user); + $user = self::validate_preferences_permissions($params['userid']); $processor = get_message_processor($name); $preferences = []; diff --git a/message/lib.php b/message/lib.php index de52fed8f90..90f7a293ef4 100644 --- a/message/lib.php +++ b/message/lib.php @@ -1034,3 +1034,69 @@ function message_output_fragment_processor_settings($args = []) { return $renderer->render_from_template('core_message/preferences_processor', $processoroutput->export_for_template($renderer)); } + +/** + * Checks if current user is allowed to edit messaging preferences of another user + * + * @param stdClass $user user whose preferences we are updating + * @return bool + */ +function core_message_can_edit_message_profile($user) { + global $USER; + if ($user->id == $USER->id) { + return has_capability('moodle/user:editownmessageprofile', context_system::instance()); + } else { + $personalcontext = context_user::instance($user->id); + if (!has_capability('moodle/user:editmessageprofile', $personalcontext)) { + return false; + } + if (isguestuser($user)) { + return false; + } + // No editing of admins by non-admins. + if (is_siteadmin($user) and !is_siteadmin($USER)) { + return false; + } + return true; + } +} + +/** + * Implements callback user_preferences, whitelists preferences that users are allowed to update directly + * + * Used in {@see core_user::fill_preferences_cache()}, see also {@see useredit_update_user_preference()} + * + * @return array + */ +function core_message_user_preferences() { + + $preferences = []; + $preferences['message_blocknoncontacts'] = array('type' => PARAM_INT, 'null' => NULL_NOT_ALLOWED, 'default' => 0, + 'choices' => array(0, 1)); + $preferences['/^message_provider_([\w\d_]*)_logged(in|off)$/'] = array('isregex' => true, 'type' => PARAM_NOTAGS, + 'null' => NULL_NOT_ALLOWED, 'default' => 'none', + 'permissioncallback' => function ($user, $preferencename) { + global $CFG; + require_once($CFG->libdir.'/messagelib.php'); + if (core_message_can_edit_message_profile($user) && + preg_match('/^message_provider_([\w\d_]*)_logged(in|off)$/', $preferencename, $matches)) { + $providers = message_get_providers_for_user($user->id); + foreach ($providers as $provider) { + if ($matches[1] === $provider->component . '_' . $provider->name) { + return true; + } + } + } + return false; + }, + 'cleancallback' => function ($value, $preferencename) { + if ($value === 'none' || empty($value)) { + return 'none'; + } + $parts = explode('/,/', $value); + $processors = array_keys(get_message_processors()); + array_filter($parts, function($v) use ($processors) {return in_array($v, $processors);}); + return $parts ? join(',', $parts) : 'none'; + }); + return $preferences; +} diff --git a/message/output/email/message_output_email.php b/message/output/email/message_output_email.php index 196e353ba79..1a7a222d33b 100644 --- a/message/output/email/message_output_email.php +++ b/message/output/email/message_output_email.php @@ -164,10 +164,13 @@ class message_output_email extends message_output { global $CFG; if (isset($form->email_email)) { - $preferences['message_processor_email_email'] = $form->email_email; + $preferences['message_processor_email_email'] = clean_param($form->email_email, PARAM_EMAIL); } if (isset($form->preference_mailcharset)) { $preferences['mailcharset'] = $form->preference_mailcharset; + if (!array_key_exists($preferences['mailcharset'], get_list_of_charsets())) { + $preferences['mailcharset'] = '0'; + } } if (isset($form->mailformat) && isset($form->userid)) { require_once($CFG->dirroot.'/user/lib.php'); diff --git a/user/calendar.php b/user/calendar.php index 486d5d63c28..ef810f4dd54 100644 --- a/user/calendar.php +++ b/user/calendar.php @@ -58,30 +58,14 @@ if ($calendarform->is_cancelled()) { } else if ($calendarform->is_submitted() && $calendarform->is_validated() && confirm_sesskey()) { $data = $calendarform->get_data(); - // Time format. - if ($data->timeformat != CALENDAR_TF_12 && $data->timeformat != CALENDAR_TF_24) { - $data->timeformat = ''; - } - set_user_preference('calendar_timeformat', $data->timeformat); - - // Start weekday. - $data->startwday = intval($data->startwday); - if ($data->startwday < 0 || $data->startwday > 6) { - $data->startwday = abs($data->startwday % 7); - } - set_user_preference('calendar_startwday', $data->startwday); - - // Calendar events. - if (intval($data->maxevents) >= 1) { - set_user_preference('calendar_maxevents', $data->maxevents); - } - - // Calendar lookahead. - if (intval($data->lookahead) >= 1) { - set_user_preference('calendar_lookahead', $data->lookahead); - } - - set_user_preference('calendar_persistflt', intval($data->persistflt)); + $usernew = ['id' => $USER->id, + 'preference_calendar_timeformat' => $data->timeformat, + 'preference_calendar_startwday' => $data->startwday, + 'preference_calendar_maxevents' => $data->maxevents, + 'preference_calendar_lookahead' => $data->lookahead, + 'preference_calendar_persistflt' => $data->persistflt + ]; + useredit_update_user_preference($usernew); // Calendar type. $calendartype = $data->calendartype; diff --git a/user/course.php b/user/course.php index d0e571b35f2..66cb0b52ea2 100644 --- a/user/course.php +++ b/user/course.php @@ -41,7 +41,8 @@ $redirect = new moodle_url("/user/preferences.php", array('userid' => $user->id) if ($courseform->is_cancelled()) { redirect($redirect); } else if ($data = $courseform->get_data()) { - set_user_preference('usemodchooser', $data->enableactivitychooser, $user); + useredit_update_user_preference(['id' => $user->id, + 'preference_usemodchooser' => $data->enableactivitychooser]); redirect($redirect); } diff --git a/user/edit.php b/user/edit.php index 8a18c4163eb..deeca38c568 100644 --- a/user/edit.php +++ b/user/edit.php @@ -199,14 +199,16 @@ if ($usernew = $userform->get_data()) { // Other users require a confirmation email. if (isset($usernew->email) and $user->email != $usernew->email && !has_capability('moodle/user:update', $systemcontext)) { $a = new stdClass(); - $a->newemail = $usernew->preference_newemail = $usernew->email; - $usernew->preference_newemailkey = random_string(20); - $usernew->preference_newemailattemptsleft = 3; + $emailchangedkey = random_string(20); + set_user_preference('newemail', $usernew->email, $user->id); + set_user_preference('newemailkey', $emailchangedkey, $user->id); + set_user_preference('newemailattemptsleft', 3, $user->id); + + $a->newemail = $emailchanged = $usernew->email; $a->oldemail = $usernew->email = $user->email; $emailchangedhtml = $OUTPUT->box(get_string('auth_changingemailaddress', 'auth', $a), 'generalbox', 'notice'); $emailchangedhtml .= $OUTPUT->continue_button($returnurl); - $emailchanged = true; } } @@ -254,14 +256,14 @@ if ($usernew = $userform->get_data()) { \core\event\user_updated::create_from_userid($user->id)->trigger(); // If email was changed and confirmation is required, send confirmation email now to the new address. - if ($emailchanged && $CFG->emailchangeconfirmation) { + if ($emailchanged !== false && $CFG->emailchangeconfirmation) { $tempuser = $DB->get_record('user', array('id' => $user->id), '*', MUST_EXIST); - $tempuser->email = $usernew->preference_newemail; + $tempuser->email = $emailchanged; $supportuser = core_user::get_support_user(); $a = new stdClass(); - $a->url = $CFG->wwwroot . '/user/emailupdate.php?key=' . $usernew->preference_newemailkey . '&id=' . $user->id; + $a->url = $CFG->wwwroot . '/user/emailupdate.php?key=' . $emailchangedkey . '&id=' . $user->id; $a->site = format_string($SITE->fullname, true, array('context' => context_course::instance(SITEID))); $a->fullname = fullname($tempuser, true); $a->supportemail = $supportuser->email; diff --git a/user/editlib.php b/user/editlib.php index 9e0a66b25b9..7c80aab5882 100644 --- a/user/editlib.php +++ b/user/editlib.php @@ -147,16 +147,38 @@ function useredit_load_preferences(&$user, $reload=true) { } /** - * Updates the user preferences for teh given user. + * Updates the user preferences for the given user * - * @param stdClass|array $usernew + * Only preference that can be updated directly will be updated here. This method is called from various WS + * updating users and should be used when updating user details. Plugins may whitelist preferences that can + * be updated by defining 'user_preferences' callback, {@see core_user::fill_preferences_cache()} + * + * Some parts of code may use user preference table to store internal data, in these cases it is acceptable + * to call set_user_preference() + * + * @param stdClass|array $usernew object or array that has user preferences as attributes with keys starting with preference_ */ function useredit_update_user_preference($usernew) { + global $USER; $ua = (array)$usernew; + if (is_object($usernew) && isset($usernew->id) && isset($usernew->deleted) && isset($usernew->confirmed)) { + // This is already a full user object, maybe not completely full but these fields are enough. + $user = $usernew; + } else if (empty($ua['id']) || $ua['id'] == $USER->id) { + // We are updating current user. + $user = $USER; + } else { + // Retrieve user object. + $user = core_user::get_user($ua['id'], '*', MUST_EXIST); + } + foreach ($ua as $key => $value) { if (strpos($key, 'preference_') === 0) { $name = substr($key, strlen('preference_')); - set_user_preference($name, $value, $usernew->id); + if (core_user::can_edit_preference($name, $user)) { + $value = core_user::clean_preference($value, $name); + set_user_preference($name, $value, $user->id); + } } } } diff --git a/user/editor.php b/user/editor.php index febb4c39a20..b1030681914 100644 --- a/user/editor.php +++ b/user/editor.php @@ -48,7 +48,7 @@ if ($editorform->is_cancelled()) { $user->preference_htmleditor = $data->preference_htmleditor; - useredit_update_user_preference($user, false, false); + useredit_update_user_preference($user); // Trigger event. \core\event\user_updated::create_from_userid($user->id)->trigger(); diff --git a/user/externallib.php b/user/externallib.php index a32605c3189..5ead62ce4e9 100644 --- a/user/externallib.php +++ b/user/externallib.php @@ -229,9 +229,11 @@ class core_user_external extends external_api { // Preferences. if (!empty($user['preferences'])) { + $userpref = (object)$user; foreach ($user['preferences'] as $preference) { - set_user_preference($preference['type'], $preference['value'], $user['id']); + $userpref->{'preference_'.$preference['type']} = $preference['value']; } + useredit_update_user_preference($userpref); } $userids[] = array('id' => $user['id'], 'username' => $user['username']); @@ -359,6 +361,8 @@ class core_user_external extends external_api { global $USER, $CFG; require_once($CFG->dirroot . '/user/lib.php'); + require_once($CFG->dirroot . '/user/editlib.php'); + require_once($CFG->dirroot . '/message/lib.php'); if (empty($userid)) { $userid = $USER->id; @@ -373,38 +377,29 @@ class core_user_external extends external_api { ); self::validate_parameters(self::update_user_preferences_parameters(), $params); - if ($userid == $USER->id) { - require_capability('moodle/user:editownmessageprofile', $systemcontext); - } else { - $personalcontext = context_user::instance($userid); - require_capability('moodle/user:editmessageprofile', $personalcontext); - // No editing of guest user account. - if (isguestuser($userid)) { - print_error('guestnoeditmessageother', 'message'); - } - // No editing of admins by non-admins. - if (is_siteadmin($userid) and !is_siteadmin($USER)) { - print_error('useradmineditadmin'); - } - } - // Preferences. if (!empty($preferences)) { + $userpref = ['id' => $userid]; foreach ($preferences as $preference) { - set_user_preference($preference['type'], $preference['value'], $userid); + $userpref['preference_' . $preference['type']] = $preference['value']; } + useredit_update_user_preference($userpref); } // Check if they want to update the email. if ($emailstop !== null) { - $user = new stdClass(); - $user->id = $userid; - $user->emailstop = $emailstop; - user_update_user($user); + $otheruser = ($userid == $USER->id) ? $USER : core_user::get_user($userid, '*', MUST_EXIST); + core_user::require_active_user($otheruser); + if (core_message_can_edit_message_profile($otheruser) && $otheruser->emailstop != $emailstop) { + $user = new stdClass(); + $user->id = $userid; + $user->emailstop = $emailstop; + user_update_user($user); - // Update the $USER if we should. - if ($userid == $USER->id) { - $USER->emailstop = $emailstop; + // Update the $USER if we should. + if ($userid == $USER->id) { + $USER->emailstop = $emailstop; + } } } @@ -521,6 +516,7 @@ class core_user_external extends external_api { global $CFG, $DB, $USER; require_once($CFG->dirroot."/user/lib.php"); require_once($CFG->dirroot."/user/profile/lib.php"); // Required for customfields related function. + require_once($CFG->dirroot.'/user/editlib.php'); // Ensure the current user is allowed to run this function. $context = context_system::instance(); @@ -582,9 +578,11 @@ class core_user_external extends external_api { // Preferences. if (!empty($user['preferences'])) { + $userpref = clone($existinguser); foreach ($user['preferences'] as $preference) { - set_user_preference($preference['type'], $preference['value'], $user['id']); + $userpref->{'preference_'.$preference['type']} = $preference['value']; } + useredit_update_user_preference($userpref); } if (isset($user['suspended']) and $user['suspended']) { \core\session\manager::kill_user_sessions($user['id']); @@ -1710,7 +1708,6 @@ class core_user_external extends external_api { $context = context_system::instance(); self::validate_context($context); - require_capability('moodle/site:config', $context); $userscache = array(); foreach ($params['preferences'] as $pref) { @@ -1734,11 +1731,21 @@ class core_user_external extends external_api { } try { - set_user_preference($pref['name'], $pref['value'], $user); - $saved[] = array( - 'name' => $pref['name'], - 'userid' => $user->id, - ); + if (core_user::can_edit_preference($pref['name'], $user)) { + $value = core_user::clean_preference($pref['value'], $pref['name']); + set_user_preference($pref['name'], $value, $user->id); + $saved[] = array( + 'name' => $pref['name'], + 'userid' => $user->id, + ); + } else { + $warnings[] = array( + 'item' => 'user', + 'itemid' => $user->id, + 'warningcode' => 'nopermission', + 'message' => 'You are not allowed to change the preference '.s($pref['name']).' for user '.$user->id + ); + } } catch (Exception $e) { $warnings[] = array( 'item' => 'user', diff --git a/user/tests/externallib_test.php b/user/tests/externallib_test.php index 16405ae184e..be52830487f 100644 --- a/user/tests/externallib_test.php +++ b/user/tests/externallib_test.php @@ -494,11 +494,19 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { 'email' => 'usertest1@example.com', 'description' => 'This is a description for user 1', 'city' => 'Perth', - 'country' => 'AU' + 'country' => 'AU', + 'preferences' => [[ + 'type' => 'htmleditor', + 'value' => 'atto' + ], [ + 'type' => 'invalidpreference', + 'value' => 'abcd' + ]] ); $context = context_system::instance(); $roleid = $this->assignUserCapability('moodle/user:create', $context->id); + $this->assignUserCapability('moodle/user:editprofile', $context->id, $roleid); // Call the external function. $createdusers = core_user_external::create_users(array($user1)); @@ -519,6 +527,8 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { $this->assertEquals($dbuser->description, $user1['description']); $this->assertEquals($dbuser->city, $user1['city']); $this->assertEquals($dbuser->country, $user1['country']); + $this->assertEquals('atto', get_user_preferences('htmleditor', null, $dbuser)); + $this->assertEquals(null, get_user_preferences('invalidpreference', null, $dbuser)); } // Call without required capability @@ -599,11 +609,19 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { 'description' => 'This is a description for user 1', 'city' => 'Perth', 'userpicture' => $draftid, - 'country' => 'AU' + 'country' => 'AU', + 'preferences' => [[ + 'type' => 'htmleditor', + 'value' => 'atto' + ], [ + 'type' => 'invialidpreference', + 'value' => 'abcd' + ]] ); $context = context_system::instance(); $roleid = $this->assignUserCapability('moodle/user:update', $context->id); + $this->assignUserCapability('moodle/user:editprofile', $context->id, $roleid); // Check we can't update deleted users, guest users, site admin. $user2 = $user3 = $user4 = $user1; @@ -636,6 +654,8 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { $this->assertEquals($dbuser->city, $user1['city']); $this->assertEquals($dbuser->country, $user1['country']); $this->assertNotEquals(0, $dbuser->picture, 'Picture must be set to the new icon itemid for this user'); + $this->assertEquals('atto', get_user_preferences('htmleditor', null, $dbuser)); + $this->assertEquals(null, get_user_preferences('invalidpreference', null, $dbuser)); // Confirm no picture change when parameter is not supplied. unset($user1['userpicture']); @@ -952,13 +972,13 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { $this->setAdminUser(); $preferences = array( array( - 'name' => 'some_random_pref', - 'value' => 'abc', + 'name' => 'htmleditor', + 'value' => 'atto', 'userid' => $user1->id, ), array( - 'name' => 'some_random_pref', - 'value' => 'def', + 'name' => 'htmleditor', + 'value' => 'tinymce', 'userid' => $user2->id, ) ); @@ -969,10 +989,39 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { $this->assertCount(2, $result['saved']); // Get preference from DB to avoid cache. - $this->assertEquals('abc', $DB->get_field('user_preferences', 'value', - array('userid' => $user1->id, 'name' => 'some_random_pref'))); - $this->assertEquals('def', $DB->get_field('user_preferences', 'value', - array('userid' => $user2->id, 'name' => 'some_random_pref'))); + $this->assertEquals('atto', $DB->get_field('user_preferences', 'value', + array('userid' => $user1->id, 'name' => 'htmleditor'))); + $this->assertEquals('tinymce', $DB->get_field('user_preferences', 'value', + array('userid' => $user2->id, 'name' => 'htmleditor'))); + } + + /** + * Test set_user_preferences + */ + public function test_set_user_preferences_save_invalid_pref() { + global $DB; + $this->resetAfterTest(true); + + $user1 = self::getDataGenerator()->create_user(); + + // Save users preferences. + $this->setAdminUser(); + $preferences = array( + array( + 'name' => 'some_random_pref', + 'value' => 'abc', + 'userid' => $user1->id, + ), + ); + + $result = core_user_external::set_user_preferences($preferences); + $result = external_api::clean_returnvalue(core_user_external::set_user_preferences_returns(), $result); + $this->assertCount(1, $result['warnings']); + $this->assertCount(0, $result['saved']); + $this->assertEquals('nopermission', $result['warnings'][0]['warningcode']); + + // Nothing was written to DB. + $this->assertEmpty($DB->count_records('user_preferences', array('name' => 'some_random_pref'))); } /** @@ -1002,7 +1051,7 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { * Test set_user_preferences using an invalid preference */ public function test_set_user_preferences_invalid_preference() { - global $USER; + global $USER, $DB; $this->resetAfterTest(true); // Create a very long value. @@ -1017,15 +1066,17 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { $result = core_user_external::set_user_preferences($preferences); $result = external_api::clean_returnvalue(core_user_external::set_user_preferences_returns(), $result); - $this->assertCount(1, $result['warnings']); - $this->assertCount(0, $result['saved']); - $this->assertEquals('errorsavingpreference', $result['warnings'][0]['warningcode']); + $this->assertCount(0, $result['warnings']); + $this->assertCount(1, $result['saved']); + // Cleaned valud of the preference was saved. + $this->assertEquals(1, $DB->get_field('user_preferences', 'value', + array('userid' => $USER->id, 'name' => 'calendar_maxevents'))); } /** * Test set_user_preferences for other user not being admin */ - public function test_set_user_preferences_other_user_not_being_admin() { + public function test_set_user_preferences_capability() { $this->resetAfterTest(true); $user1 = self::getDataGenerator()->create_user(); @@ -1040,8 +1091,12 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { ) ); - $this->expectException('required_capability_exception'); $result = core_user_external::set_user_preferences($preferences); + + $this->assertCount(1, $result['warnings']); + $this->assertCount(0, $result['saved']); + $this->assertEquals('nopermission', $result['warnings'][0]['warningcode']); + $this->assertEquals($user2->id, $result['warnings'][0]['itemid']); } /**