MDL-46099 session: fix use of references for session globals

This reverses the references used for global $USER and $SESSION, the reason is that PHP does not allow references to references. $USER is a reference to $GLOBALS['USER'] which means we cannot put any references to it. Solution is to store the current user and session objects in $GLOBALS['USER'] and $GLOBALS['SESSIOn'] are reference them in $_SESSION. This patch makes the session code behave the same way in CLI, phpunit and normal web requests - this allows use to finally unit test most aspects of the session code in Moodle.
moodle · Jun 30, 2014 · df9a669 · df9a669
1 parent 83d08fc
commit df9a669
Show file tree

Hide file tree

Showing 13 changed files with 432 additions and 93 deletions.
diff --git a/admin/cli/install.php b/admin/cli/install.php
@@ -201,17 +201,9 @@
 require($CFG->dirroot.'/version.php');
 $CFG->target_release = $release;
 
-$_SESSION = array();
-$_SESSION['SESSION'] = new stdClass();
-$_SESSION['SESSION']->lang = $CFG->lang;
-$_SESSION['USER'] = new stdClass();
-$_SESSION['USER']->id = 0;
-$_SESSION['USER']->mnethostid = 1;
-
+\core\session\manager::init_empty_session();
 global $SESSION;
 global $USER;
-$SESSION = &$_SESSION['SESSION'];
-$USER    = &$_SESSION['USER'];
 
 global $COURSE;
 $COURSE = new stdClass();

diff --git a/admin/tool/log/classes/helper/buffered_writer.php b/admin/tool/log/classes/helper/buffered_writer.php
@@ -72,7 +72,7 @@ public function write(\core\event\base $event) {
         $entry['other'] = serialize($entry['other']);
         $entry['origin'] = $PAGE->requestorigin;
         $entry['ip'] = $PAGE->requestip;
-        $entry['realuserid'] = \core\session\manager::is_loggedinas() ? $_SESSION['USER']->realuser : null;
+        $entry['realuserid'] = \core\session\manager::is_loggedinas() ? $GLOBALS['USER']->realuser : null;
 
         $this->buffer[] = $entry;
         $this->count++;

diff --git a/admin/tool/log/store/database/tests/store_test.php b/admin/tool/log/store/database/tests/store_test.php
@@ -130,8 +130,7 @@ public function test_log_writing() {
             array('context' => context_module::instance($module2->cmid), 'other' => array('sample' => 6, 'xx' => 9)));
         $event2->trigger();
 
-        $_SESSION['SESSION'] = new \stdClass();
-        $this->setUser(0);
+        \core\session\manager::init_empty_session();
         $this->assertFalse(\core\session\manager::is_loggedinas());
 
         $logs = $DB->get_records('logstore_standard_log', array(), 'id ASC');

diff --git a/admin/tool/log/store/standard/tests/store_test.php b/admin/tool/log/store/standard/tests/store_test.php
@@ -95,8 +95,7 @@ public function test_log_writing() {
         $event2->trigger();
         logstore_standard_restore::hack_executing(0);
 
-        $_SESSION['SESSION'] = new \stdClass();
-        $this->setUser(0);
+        \core\session\manager::init_empty_session();
         $this->assertFalse(\core\session\manager::is_loggedinas());
 
         $logs = $DB->get_records('logstore_standard_log', array(), 'id ASC');

diff --git a/install.php b/install.php
@@ -229,17 +229,9 @@
 require('version.php');
 $CFG->target_release = $release;
 
-$_SESSION = array();
-$_SESSION['SESSION'] = new stdClass();
-$_SESSION['SESSION']->lang = $CFG->lang;
-$_SESSION['USER'] = new stdClass();
-$_SESSION['USER']->id = 0;
-$_SESSION['USER']->mnethostid = 1;
-
+\core\session\manager::init_empty_session();
 global $SESSION;
 global $USER;
-$SESSION = &$_SESSION['SESSION'];
-$USER    = &$_SESSION['USER'];
 
 global $COURSE;
 $COURSE = new stdClass();

diff --git a/lib/classes/session/manager.php b/lib/classes/session/manager.php
@@ -79,6 +79,16 @@ public static function start() {
             self::initialise_user_session($newsid);
             self::check_security();
 
+            // Link global $USER and $SESSION,
+            // this is tricky because PHP does not allow references to references
+            // and global keyword uses internally once reference to the $GLOBALS array.
+            // The solution is to use the $GLOBALS['USER'] and $GLOBALS['$SESSION']
+            // as the main storage of data and put references to $_SESSION.
+            $GLOBALS['USER'] = $_SESSION['USER'];
+            $_SESSION['USER'] =& $GLOBALS['USER'];
+            $GLOBALS['SESSION'] = $_SESSION['SESSION'];
+            $_SESSION['SESSION'] =& $GLOBALS['SESSION'];
+
         } catch (\Exception $ex) {
             @session_write_close();
             self::init_empty_session();
@@ -139,31 +149,29 @@ protected static function load_handler() {
 
     /**
      * Empty current session, fill it with not-logged-in user info.
+     *
+     * This is intended for installation scripts, unit tests and other
+     * special areas. Do NOT use for logout and session termination
+     * in normal requests!
      */
-    protected static function init_empty_session() {
+    public static function init_empty_session() {
         global $CFG;
 
-        // Session not used at all.
-        $_SESSION = array();
-        $_SESSION['SESSION']  = new \stdClass();
-        $_SESSION['USER']     = new \stdClass();
-        $_SESSION['USER']->id = 0;
+        $GLOBALS['SESSION'] = new \stdClass();
+
+        $GLOBALS['USER'] = new \stdClass();
+        $GLOBALS['USER']->id = 0;
         if (isset($CFG->mnet_localhost_id)) {
-            $_SESSION['USER']->mnethostid = $CFG->mnet_localhost_id;
+            $GLOBALS['USER']->mnethostid = $CFG->mnet_localhost_id;
         } else {
             // Not installed yet, the future host id will be most probably 1.
-            $_SESSION['USER']->mnethostid = 1;
+            $GLOBALS['USER']->mnethostid = 1;
         }
 
-        if (PHPUNIT_TEST or defined('BEHAT_TEST')) {
-            // Phpunit tests and behat init use reversed reference,
-            // the reason is we can not point global to $_SESSION outside of global scope.
-            global $USER, $SESSION;
-            $USER = $_SESSION['USER'];
-            $SESSION = $_SESSION['SESSION'];
-            $_SESSION['USER'] =& $USER;
-            $_SESSION['SESSION'] =& $SESSION;
-        }
+        // Link global $USER and $SESSION.
+        $_SESSION = array();
+        $_SESSION['USER'] =& $GLOBALS['USER'];
+        $_SESSION['SESSION'] =& $GLOBALS['SESSION'];
     }
 
     /**
@@ -249,9 +257,11 @@ protected static function prepare_cookies() {
     }
 
     /**
-     * Initialise $USER and $SESSION objects, handles google access
+     * Initialise $_SESSION, handles google access
      * and sets up not-logged-in user properly.
      *
+     * WARNING: $USER and $SESSION are set up later, do not use them yet!
+     *
      * @param bool $newsid is this a new session in first http request?
      */
     protected static function initialise_user_session($newsid) {
@@ -416,6 +426,8 @@ protected static function add_session_record($userid) {
 
     /**
      * Do various session security checks.
+     *
+     * WARNING: $USER and $SESSION are set up later, do not use them yet!
      */
     protected static function check_security() {
         global $CFG;
@@ -489,7 +501,7 @@ public static function terminate_current() {
         session_regenerate_id(true);
         $DB->delete_records('sessions', array('sid'=>$sid));
         self::init_empty_session();
-        self::add_session_record($_SESSION['USER']->id);
+        self::add_session_record($_SESSION['USER']->id); // Do not use $USER here because it may not be set up yet.
         session_write_close();
         self::$sessionactive = false;
     }
@@ -590,22 +602,19 @@ public static function kill_user_sessions($userid) {
      * @param \stdClass $user record
      */
     public static function set_user(\stdClass $user) {
-        $_SESSION['USER'] = $user;
-        unset($_SESSION['USER']->description); // Conserve memory.
-        unset($_SESSION['USER']->password);    // Improve security.
-        if (isset($_SESSION['USER']->lang)) {
+        $GLOBALS['USER'] = $user;
+        unset($GLOBALS['USER']->description); // Conserve memory.
+        unset($GLOBALS['USER']->password);    // Improve security.
+        if (isset($GLOBALS['USER']->lang)) {
             // Make sure it is a valid lang pack name.
-            $_SESSION['USER']->lang = clean_param($_SESSION['USER']->lang, PARAM_LANG);
+            $GLOBALS['USER']->lang = clean_param($GLOBALS['USER']->lang, PARAM_LANG);
         }
-        sesskey(); // Init session key.
 
-        if (PHPUNIT_TEST or defined('BEHAT_TEST')) {
-            // Phpunit tests and behat init use reversed reference,
-            // the reason is we can not point global to $_SESSION outside of global scope.
-            global $USER;
-            $USER = $_SESSION['USER'];
-            $_SESSION['USER'] =& $USER;
-        }
+        // Relink session with global $USER just in case it got unlinked somehow.
+        $_SESSION['USER'] =& $GLOBALS['USER'];
+
+        // Init session key.
+        sesskey();
     }
 
     /**
@@ -697,7 +706,7 @@ public static function gc() {
      * @return bool
      */
     public static function is_loggedinas() {
-        return !empty($_SESSION['USER']->realuser);
+        return !empty($GLOBALS['USER']->realuser);
     }
 
     /**
@@ -708,7 +717,7 @@ public static function get_realuser() {
         if (self::is_loggedinas()) {
             return $_SESSION['REALUSER'];
         } else {
-            return $_SESSION['USER'];
+            return $GLOBALS['USER'];
         }
     }
 
@@ -725,12 +734,14 @@ public static function loginas($userid, \context $context) {
             return;
         }
 
-        // Switch to fresh new $SESSION.
-        $_SESSION['REALSESSION'] = $_SESSION['SESSION'];
-        $_SESSION['SESSION']     = new \stdClass();
+        // Switch to fresh new $_SESSION.
+        $_SESSION = array();
+        $_SESSION['REALSESSION'] = clone($GLOBALS['SESSION']);
+        $GLOBALS['SESSION'] = new \stdClass();
+        $_SESSION['SESSION'] =& $GLOBALS['SESSION'];
 
         // Create the new $USER object with all details and reload needed capabilities.
-        $_SESSION['REALUSER'] = $_SESSION['USER'];
+        $_SESSION['REALUSER'] = clone($GLOBALS['USER']);
         $user = get_complete_user_data('id', $userid);
         $user->realuser       = $_SESSION['REALUSER']->id;
         $user->loginascontext = $context;

diff --git a/lib/phpunit/classes/util.php b/lib/phpunit/classes/util.php
@@ -189,14 +189,9 @@ public static function reset_all_data($detectchanges = false) {
         $FULLME = null;
         $ME = null;
         $SCRIPT = null;
-        $SESSION = new stdClass();
-        $_SESSION['SESSION'] =& $SESSION;
-
-        // set fresh new not-logged-in user
-        $user = new stdClass();
-        $user->id = 0;
-        $user->mnethostid = $CFG->mnet_localhost_id;
-        \core\session\manager::set_user($user);
+
+        // Empty sessison and set fresh new not-logged-in user.
+        \core\session\manager::init_empty_session();
 
         // reset all static caches
         \core\event\manager::phpunit_reset();

diff --git a/lib/phpunit/tests/advanced_test.php b/lib/phpunit/tests/advanced_test.php
@@ -76,33 +76,39 @@ public function test_set_user() {
 
         $this->assertEquals(0, $USER->id);
         $this->assertSame($_SESSION['USER'], $USER);
+        $this->assertSame($GLOBALS['USER'], $USER);
 
         $user = $DB->get_record('user', array('id'=>2));
         $this->assertNotEmpty($user);
         $this->setUser($user);
         $this->assertEquals(2, $USER->id);
         $this->assertEquals(2, $_SESSION['USER']->id);
         $this->assertSame($_SESSION['USER'], $USER);
+        $this->assertSame($GLOBALS['USER'], $USER);
 
         $USER->id = 3;
         $this->assertEquals(3, $USER->id);
         $this->assertEquals(3, $_SESSION['USER']->id);
         $this->assertSame($_SESSION['USER'], $USER);
+        $this->assertSame($GLOBALS['USER'], $USER);
 
         \core\session\manager::set_user($user);
         $this->assertEquals(2, $USER->id);
         $this->assertEquals(2, $_SESSION['USER']->id);
         $this->assertSame($_SESSION['USER'], $USER);
+        $this->assertSame($GLOBALS['USER'], $USER);
 
         $USER = $DB->get_record('user', array('id'=>1));
         $this->assertNotEmpty($USER);
         $this->assertEquals(1, $USER->id);
         $this->assertEquals(1, $_SESSION['USER']->id);
         $this->assertSame($_SESSION['USER'], $USER);
+        $this->assertSame($GLOBALS['USER'], $USER);
 
         $this->setUser(null);
         $this->assertEquals(0, $USER->id);
         $this->assertSame($_SESSION['USER'], $USER);
+        $this->assertSame($GLOBALS['USER'], $USER);
     }
 
     public function test_set_admin_user() {

diff --git a/lib/sessionlib.php b/lib/sessionlib.php
@@ -37,7 +37,10 @@ function sesskey() {
     // note: do not use $USER because it may not be initialised yet
     if (empty($_SESSION['USER']->sesskey)) {
         if (!isset($_SESSION['USER'])) {
-            $_SESSION['USER'] = new stdClass;
+            // This should never happen,
+            // do not mess with session and globals here,
+            // let any checks fail instead!
+            return false;
         }
         $_SESSION['USER']->sesskey = random_string(10);
     }
@@ -151,16 +154,28 @@ function get_moodle_cookie() {
  * Sets up current user and course environment (lang, etc.) in cron.
  * Do not use outside of cron script!
  *
- * @param stdClass $user full user object, null means default cron user (admin)
- * @param $course full course record, null means $SITE
+ * @param stdClass $user full user object, null means default cron user (admin),
+ *                 value 'reset' means reset internal static caches.
+ * @param stdClass $course full course record, null means $SITE
  * @return void
  */
 function cron_setup_user($user = NULL, $course = NULL) {
     global $CFG, $SITE, $PAGE;
 
+    if (!CLI_SCRIPT) {
+        throw new coding_exception('Function cron_setup_user() cannot be used in normal requests!');
+    }
+
     static $cronuser    = NULL;
     static $cronsession = NULL;
 
+    if ($user === 'reset') {
+        $cronuser = null;
+        $cronsession = null;
+        \core\session\manager::init_empty_session();
+        return;
+    }
+
     if (empty($cronuser)) {
         /// ignore admins timezone, language and locale - use site default instead!
         $cronuser = get_admin();
@@ -173,15 +188,16 @@ function cron_setup_user($user = NULL, $course = NULL) {
     }
 
     if (!$user) {
-        // cached default cron user (==modified admin for now)
+        // Cached default cron user (==modified admin for now).
+        \core\session\manager::init_empty_session();
         \core\session\manager::set_user($cronuser);
-        $_SESSION['SESSION'] = $cronsession;
+        $GLOBALS['SESSION'] = $cronsession;
 
     } else {
-        // emulate real user session - needed for caps in cron
-        if ($_SESSION['USER']->id != $user->id) {
+        // Emulate real user session - needed for caps in cron.
+        if ($GLOBALS['USER']->id != $user->id) {
+            \core\session\manager::init_empty_session();
             \core\session\manager::set_user($user);
-            $_SESSION['SESSION'] = new stdClass();
         }
     }
 

diff --git a/lib/setup.php b/lib/setup.php
@@ -773,10 +773,6 @@
     $CFG->sessiontimeout = 7200;
 }
 \core\session\manager::start();
-if (!PHPUNIT_TEST and !defined('BEHAT_TEST')) {
-    $SESSION =& $_SESSION['SESSION'];
-    $USER    =& $_SESSION['USER'];
-}
 
 // Initialise some variables that are supposed to be set in config.php only.
 if (!isset($CFG->filelifetime)) {

diff --git a/lib/tests/behat/behat_hooks.php b/lib/tests/behat/behat_hooks.php
@@ -189,9 +189,7 @@ public function before_scenario($event) {
         }
 
         // Reset $SESSION.
-        $_SESSION = array();
-        $SESSION = new stdClass();
-        $_SESSION['SESSION'] =& $SESSION;
+        \core\session\manager::init_empty_session();
 
         behat_util::reset_database();
         behat_util::reset_dataroot();