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.

diff --git a/course/switchrole.php b/course/switchrole.php
index 2b756b6..533537a 100644 (file)
--- a/course/switchrole.php
+++ b/course/switchrole.php
@@ -1,5 +1,4 @@
 <?php
-
 // This file is part of Moodle - http://moodle.org/
 //
 // Moodle is free software: you can redistribute it and/or modify
@@ -20,7 +19,7 @@
  * back to the page that they were on.
  *
  * This functionality is also supported in {@link /course/view.php} in order to comply
- * with backwards compatibility
+ * with backwards compatibility.
  * The reason that we created this file was so that user didn't get redirected back
  * to the course view page only to be redirected again.
  *
@@ -34,54 +33,43 @@ require_once('../config.php');
 require_once($CFG->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 00c6754..345b1b1 100644 (file)
--- 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 8458475..1e48ff1 100644 (file)
--- 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 8e57f28..eec3b12 100644 (file)
--- 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 .= " (<a href=\"$CFG->wwwroot/course/view.php?id=$course->id&amp;switchrole=0&amp;sesskey=".sesskey()."\">".get_string('switchrolereturn').'</a>)';
+                    $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);