From aae028d9e3decb3671d656df5fbb648724a6095a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20S=CC=8Ckoda?= Date: Fri, 7 Dec 2012 18:13:22 +0100 Subject: [PATCH] MDL-36988 cleanup role switching This patch uses local URLs in parameters because it is more compatible with various security hacks. Session key is not added automatically to return url for safety reasons - we do not want to execute some random action again. POST pages are now returned to course page too. --- course/switchrole.php | 50 ++++++++++++++++------------------------- course/view.php | 2 +- lib/navigationlib.php | 4 +--- lib/outputrenderers.php | 3 ++- 4 files changed, 23 insertions(+), 36 deletions(-) diff --git a/course/switchrole.php b/course/switchrole.php index 2b756b6b745..533537a0827 100644 --- a/course/switchrole.php +++ b/course/switchrole.php @@ -1,5 +1,4 @@ dirroot.'/course/lib.php'); $id = required_param('id', PARAM_INT); -$switchrole = optional_param('switchrole',-1, PARAM_INT); -$returnurl = optional_param('returnurl', false, PARAM_LOCALURL); +$switchrole = optional_param('switchrole', -1, PARAM_INT); +$returnurl = optional_param('returnurl', '', PARAM_RAW); + +if (strpos($returnurl, '?') === false) { + // Looks like somebody did not set proper page url, better go to course page. + $returnurl = new moodle_url('/course/view.php', array('id' => $id)); +} else { + if (strpos($returnurl, $CFG->wwwroot) !== 0) { + $returnurl = $CFG->wwwroot.$returnurl; + } + $returnurl = clean_param($returnurl, PARAM_URL); +} $PAGE->set_url('/course/switchrole.php', array('id'=>$id)); -if (!confirm_sesskey()) { - print_error('confirmsesskeybad', 'error'); -} +require_sesskey(); -if (! ($course = $DB->get_record('course', array('id'=>$id)))) { - print_error('invalidcourseid', 'error'); +if (!$course = $DB->get_record('course', array('id'=>$id))) { + redirect(new moodle_url('/')); } $context = context_course::instance($course->id); -// Remove any switched roles before checking login +// Remove any switched roles before checking login. if ($switchrole == 0) { - role_switch($switchrole, $context); + role_switch(0, $context); } require_login($course); // Switchrole - sanity check in cost-order... if ($switchrole > 0 && has_capability('moodle/role:switchroles', $context)) { - // is this role assignable in this context? + // Is this role assignable in this context? // inquiring minds want to know... $aroles = get_switchable_roles($context); if (is_array($aroles) && isset($aroles[$switchrole])) { role_switch($switchrole, $context); - // Double check that this role is allowed here - require_login($course); } } -// TODO: Using SESSION->returnurl is deprecated and should be removed in the future. -// Till then this code remains to support any external applications calling this script. -if (!empty($returnurl) && is_numeric($returnurl)) { - $returnurl = false; - if (!empty($SESSION->returnurl) && strpos($SESSION->returnurl, 'moodle_url')!==false) { - debugging('Code calling switchrole should be passing a URL as a param.', DEBUG_DEVELOPER); - $returnurl = @unserialize($SESSION->returnurl); - if (!($returnurl instanceof moodle_url)) { - $returnurl = false; - } - } -} - -if ($returnurl === false) { - $returnurl = new moodle_url('/course/view.php', array('id' => $course->id)); -} - redirect($returnurl); diff --git a/course/view.php b/course/view.php index 00c675464a1..345b1b138e4 100644 --- a/course/view.php +++ b/course/view.php @@ -18,7 +18,7 @@ $section = optional_param('section', 0, PARAM_INT); $move = optional_param('move', 0, PARAM_INT); $marker = optional_param('marker',-1 , PARAM_INT); - $switchrole = optional_param('switchrole',-1, PARAM_INT); + $switchrole = optional_param('switchrole',-1, PARAM_INT); // Deprecated, use course/switchrole.php instead. $modchooser = optional_param('modchooser', -1, PARAM_BOOL); $return = optional_param('return', 0, PARAM_LOCALURL); diff --git a/lib/navigationlib.php b/lib/navigationlib.php index 84584750399..1e48ff13f2e 100644 --- a/lib/navigationlib.php +++ b/lib/navigationlib.php @@ -3595,10 +3595,8 @@ class settings_navigation extends navigation_node { if ((count($roles)==1 && array_key_exists(0, $roles))|| $assumedrole!==false) { $switchroles->force_open(); } - $returnurl = $this->page->url; - $returnurl->param('sesskey', sesskey()); foreach ($roles as $key => $name) { - $url = new moodle_url('/course/switchrole.php', array('id'=>$course->id,'sesskey'=>sesskey(), 'switchrole'=>$key, 'returnurl'=>$returnurl->out(false))); + $url = new moodle_url('/course/switchrole.php', array('id'=>$course->id, 'sesskey'=>sesskey(), 'switchrole'=>$key, 'returnurl'=>$this->page->url->out_as_local_url(false))); $switchroles->add($name, $url, self::TYPE_SETTING, null, $key, new pix_icon('i/switchrole', '')); } } diff --git a/lib/outputrenderers.php b/lib/outputrenderers.php index 8e57f287bd6..eec3b125442 100644 --- a/lib/outputrenderers.php +++ b/lib/outputrenderers.php @@ -577,7 +577,8 @@ class core_renderer extends renderer_base { } $loggedinas = get_string('loggedinas', 'moodle', $username).$rolename; if ($withlinks) { - $loggedinas .= " (wwwroot/course/view.php?id=$course->id&switchrole=0&sesskey=".sesskey()."\">".get_string('switchrolereturn').')'; + $url = new moodle_url('/course/switchrole.php', array('id'=>$course->id,'sesskey'=>sesskey(), 'switchrole'=>0, 'returnurl'=>$this->page->url->out_as_local_url(false))); + $loggedinas .= '('.html_writer::tag('a', get_string('switchrolereturn'), array('href'=>$url)).')'; } } else { $loggedinas = $realuserinfo.get_string('loggedinas', 'moodle', $username);