Merge branch 'MDL-58018_master-arraycomparison' of git://github.com/mdjnelson/moodle

config-dist.php

@@ -628,6 +628,13 @@ $CFG->admin = 'admin';
 //
 //      $CFG->debugsessionlock = 5;
 //
+// There are times when a session lock is not required during a request. For a page/service to opt-in whether or not a
+// session lock is required this setting must first be set to 'true'.
+// This is an experimental issue. The session store can not be in the session, please
+// see https://docs.moodle.org/en/Session_handling#Read_only_sessions.
+//
+//      $CFG->enable_read_only_sessions = true;
+//
 // Uninstall plugins from CLI only. This stops admins from uninstalling plugins from the graphical admin
 // user interface, and forces plugins to be uninstalled from the Command Line tool only, found at
 // admin/cli/plugin_uninstall.php.

lib/ajax/getnavbranch.php

@@ -26,6 +26,7 @@
  */
 
 define('AJAX_SCRIPT', true);
+define('READ_ONLY_SESSION', true);
 
 /** Include config */
 require_once(__DIR__ . '/../../config.php');

lib/ajax/service.php

@@ -28,6 +28,8 @@
  */
 
 define('AJAX_SCRIPT', true);
+// Services can declare 'readonlysession' in their config located in db/services.php, if not present will default to false.
+define('READ_ONLY_SESSION', true);
 
 if (!empty($_GET['nosessionupdate'])) {
     define('NO_SESSION_UPDATE', true);

lib/classes/session/database.php

@@ -172,7 +172,9 @@ class database extends handler {
             }
             if (!$this->recordid) {
                 // Lock session if exists and not already locked.
-                $this->database->get_session_lock($record->id, $this->acquiretimeout);
+                if ($this->requires_write_lock()) {
+                    $this->database->get_session_lock($record->id, $this->acquiretimeout);
+                }
                 $this->recordid = $record->id;
             }
         } catch (\dml_sessionwait_exception $ex) {

lib/classes/session/handler.php

@@ -34,6 +34,9 @@ defined('MOODLE_INTERNAL') || die();
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 abstract class handler {
+    /** @var boolean $requireswritelock does the session need and/or have a lock? */
+    protected $requireswritelock = false;
+
     /**
      * Start the session.
      * @return bool success
@@ -42,6 +45,49 @@ abstract class handler {
         return session_start();
     }
 
+    /**
+     * Write the session and release lock. If the session was not intentionally opened
+     * with a write lock, then we will abort the session instead if able.
+     */
+    public function write_close() {
+        if ($this->requires_write_lock()) {
+            session_write_close();
+            $this->requireswritelock = false;
+        } else {
+            $this->abort();
+        }
+    }
+
+    /**
+     * Release lock on the session without writing it.
+     * May not be possible in older versions of PHP. If so, session may be written anyway
+     * so that any locks are released.
+     */
+    public function abort() {
+        session_abort();
+        $this->requireswritelock = false;
+    }
+
+    /**
+     * This is called after init() and before start() to indicate whether the session
+     * opened should be writable or not. This is intentionally captured even if your
+     * handler doesn't support non-locking sessions, so that behavior (upon session close)
+     * matches closely between handlers.
+     * @param bool $requireswritelock true if needs to be open for writing
+     */
+    public function set_requires_write_lock($requireswritelock) {
+        $this->requireswritelock = $requireswritelock;
+    }
+
+    /**
+     * Has this session been opened with a writelock? Your handler should call this during
+     * start() if you support read-only sessions.
+     * @return bool true if session is intended to have a write lock.
+     */
+    public function requires_write_lock() {
+        return $this->requireswritelock;
+    }
+
     /**
      * Init session handler.
      */

lib/classes/session/manager.php

@@ -57,6 +57,27 @@ class manager {
     /** @var string $logintokenkey Key used to get and store request protection for login form. */
     protected static $logintokenkey = 'core_auth_login';
 
+    /** @var array Stores the the SESSION before a request is performed, used to check incorrect read-only modes */
+    private static $priorsession = [];
+
+    /**
+     * If the current session is not writeable, abort it, and re-open it
+     * requesting (and blocking) until a write lock is acquired.
+     * If current session was already opened with an intentional write lock,
+     * this call will not do anything.
+     * NOTE: Even when using a session handler that does not support non-locking sessions,
+     * if the original session was not opened with the explicit intention of being locked,
+     * this will still restart your session so that code behaviour matches as closely
+     * as practical across environments.
+     */
+    public static function restart_with_write_lock() {
+        if (self::$sessionactive && !self::$handler->requires_write_lock()) {
+            @self::$handler->abort();
+            self::$sessionactive = false;
+            self::start_session(true);
+        }
+    }
+
     /**
      * Start user session.
      *
@@ -82,8 +103,26 @@ class manager {
             return;
         }
 
+        if (defined('READ_ONLY_SESSION') && !empty($CFG->enable_read_only_sessions)) {
+            $requireslock = !READ_ONLY_SESSION;
+        } else {
+            $requireslock = true; // For backwards compatibility, we default to assuming that a lock is needed.
+        }
+        self::start_session($requireslock);
+    }
+
+    /**
+     * Handles starting a session.
+     *
+     * @param bool $requireslock If this is false then no write lock will be acquired,
+     *                           and the session will be read-only.
+     */
+    private static function start_session(bool $requireslock) {
+        global $PERF;
+
         try {
             self::$handler->init();
+            self::$handler->set_requires_write_lock($requireslock);
             self::prepare_cookies();
             $isnewsession = empty($_COOKIE[session_name()]);
 
@@ -99,6 +138,10 @@ class manager {
             self::$sessionactive = true; // Set here, so the session can be cleared if the security check fails.
             self::check_security();
 
+            if (!$requireslock) {
+                self::$priorsession = (array) $_SESSION['SESSION'];
+            }
+
             // 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.
@@ -633,9 +676,7 @@ class manager {
         $DB->delete_records('sessions', array('sid'=>$sid));
         self::init_empty_session();
         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;
-
+        self::write_close();
         self::append_samesite_cookie_attribute();
     }
 
@@ -655,27 +696,46 @@ class manager {
             $PERF->sessionlock['url'] = me();
             self::update_recent_session_locks($PERF->sessionlock);
             self::sessionlock_debugging();
+
+            if (!self::$handler->requires_write_lock()) {
+                // Compare the array of the earlier session data with the array now, if
+                // there is a difference then a lock is required.
+                $arraydiff = self::array_session_diff(
+                    self::$priorsession,
+                    (array) $_SESSION['SESSION']
+                );
+
+                if ($arraydiff) {
+                    if (isset($arraydiff['cachestore_session'])) {
+                        throw new \moodle_exception('The session store can not be in the session when '
+                            . 'enable_read_only_sessions is enabled');
+                    }
+
+                    error_log('This session was started as a read-only session but writes have been detected.');
+                    error_log('The following SESSION params were either added, or were updated.');
+                    foreach ($arraydiff as $key => $value) {
+                        error_log('SESSION key: ' . $key);
+                    }
+                }
+            }
         }
 
-        if (version_compare(PHP_VERSION, '5.6.0', '>=')) {
-            // More control over whether session data
-            // is persisted or not.
-            if (self::$sessionactive && session_id()) {
-                // Write session and release lock only if
-                // indication session start was clean.
-                session_write_close();
-            } else {
-                // Otherwise, if possibile lock exists want
-                // to clear it, but do not write session.
-                @session_abort();
-            }
+        // More control over whether session data
+        // is persisted or not.
+        if (self::$sessionactive && session_id()) {
+            // Write session and release lock only if
+            // indication session start was clean.
+            self::$handler->write_close();
         } else {
-            // Any indication session was started, attempt
-            // to close it.
-            if (self::$sessionactive || session_id()) {
-                session_write_close();
+            // Otherwise, if possible lock exists want
+            // to clear it, but do not write session.
+            // If the $handler has not been set then
+            // there is no session to abort.
+            if (isset(self::$handler)) {
+                @self::$handler->abort();
             }
         }
+
         self::$sessionactive = false;
     }
 
@@ -1317,4 +1377,27 @@ class manager {
             }
         }
     }
+
+    /**
+     * Compares two arrays outputs the difference.
+     *
+     * Note this does not use array_diff_assoc due to
+     * the use of stdClasses in Moodle sessions.
+     *
+     * @param array $array1
+     * @param array $array2
+     * @return array
+     */
+    private static function array_session_diff(array $array1, array $array2) : array {
+        $difference = [];
+        foreach ($array1 as $key => $value) {
+            if (!isset($array2[$key])) {
+                $difference[$key] = $value;
+            } else if ($array2[$key] !== $value) {
+                $difference[$key] = $value;
+            }
+        }
+
+        return $difference;
+    }
 }

lib/classes/session/memcached.php

@@ -101,6 +101,8 @@ class memcached extends handler {
      * @return bool success
      */
     public function start() {
+        ini_set('memcached.sess_locking', $this->requires_write_lock() ? '1' : '0');
+
         // NOTE: memcached before 2.2.0 expires session locks automatically after max_execution_time,
         //       this leads to major difference compared to other session drivers that timeout
         //       and stop the second request execution instead.
@@ -148,7 +150,6 @@ class memcached extends handler {
         ini_set('session.save_handler', 'memcached');
         ini_set('session.save_path', $this->savepath);
         ini_set('memcached.sess_prefix', $this->prefix);
-        ini_set('memcached.sess_locking', '1'); // Locking is required!
         ini_set('memcached.sess_lock_expire', $this->lockexpire);
 
         if (version_compare($version, '3.0.0-dev') >= 0) {

lib/classes/session/redis.php

@@ -255,9 +255,11 @@ class redis extends handler {
      */
     public function handler_read($id) {
         try {
-            $this->lock_session($id);
+            if ($this->requires_write_lock()) {
+                $this->lock_session($id);
+            }
             $sessiondata = $this->connection->get($id);
-            if ($sessiondata === false) {
+            if ($sessiondata === false && $this->requires_write_lock()) {
                 $this->unlock_session($id);
                 return '';
             }

lib/db/services.php

@@ -748,6 +748,7 @@ $functions = array(
         'type' => 'read',
         'loginrequired' => false,
         'ajax' => true,
+        'readonlysession' => false, // Fetching removes from stack.
     ),
     'core_session_touch' => array(
         'classname' => 'core\session\external',
@@ -1374,6 +1375,7 @@ $functions = array(
         'type' => 'read',
         'ajax' => true,
         'services' => array(MOODLE_OFFICIAL_MOBILE_SERVICE),
+        'readonlysession' => true, // We don't modify the session.
     ),
     'core_message_mark_all_notifications_as_read' => array(
         'classname' => 'core_message_external',

lib/externallib.php

@@ -161,6 +161,11 @@ class external_api {
             } else {
                 $function->loginrequired = true;
             }
+            if (isset($functions[$function->name]['readonlysession'])) {
+                $function->readonlysession = $functions[$function->name]['readonlysession'];
+            } else {
+                $function->readonlysession = false;
+            }
         }
 
         return $function;
@@ -184,6 +189,12 @@ class external_api {
 
         $externalfunctioninfo = static::external_function_info($function);
 
+        // Eventually this should shift into the various handlers and not be handled via config.
+        $readonlysession = $externalfunctioninfo->readonlysession ?? false;
+        if (!$readonlysession || empty($CFG->enable_read_only_sessions)) {
+            \core\session\manager::restart_with_write_lock();
+        }
+
         $currentpage = $PAGE;
         $currentcourse = $COURSE;
         $response = array();

lib/tests/session_manager_test.php

@@ -854,4 +854,61 @@ class core_session_manager_testcase extends advanced_testcase {
         $this->assertCount(1, $SESSION->recentsessionlocks);
         $this->assertEquals('/good.php?id=4', $SESSION->recentsessionlocks[0]['url']);
     }
+
+    public function test_array_session_diff_same_array() {
+        $a = [];
+        $a['c'] = new stdClass();
+        $a['c']->o = new stdClass();
+        $a['c']->o->o = new stdClass();
+        $a['c']->o->o->l = 'cool';
+
+        $class = new ReflectionClass('\core\session\manager');
+        $method = $class->getMethod('array_session_diff');
+        $method->setAccessible(true);
+
+        $result = $method->invokeArgs(null, [$a, $a]);
+
+        $this->assertEmpty($result);
+    }
+
+    public function test_array_session_diff_first_array_larger() {
+        $a = [];
+        $a['stdClass'] = new stdClass();
+        $a['stdClass']->attribute = 'This is an attribute';
+        $a['array'] = ['array', 'contents'];
+
+        $b = [];
+        $b['array'] = ['array', 'contents'];
+
+        $class = new ReflectionClass('\core\session\manager');
+        $method = $class->getMethod('array_session_diff');
+        $method->setAccessible(true);
+
+        $result = $method->invokeArgs(null, [$a, $b]);
+
+        $expected = [];
+        $expected['stdClass'] = new stdClass();
+        $expected['stdClass']->attribute = 'This is an attribute';
+        $this->assertEquals($expected, $result);
+    }
+
+    public function test_array_session_diff_second_array_larger() {
+        $a = [];
+        $a['array'] = ['array', 'contents'];
+
+        $b = [];
+        $b['stdClass'] = new stdClass();
+        $b['stdClass']->attribute = 'This is an attribute';
+        $b['array'] = ['array', 'contents'];
+
+        $class = new ReflectionClass('\core\session\manager');
+        $method = $class->getMethod('array_session_diff');
+        $method->setAccessible(true);
+
+        $result = $method->invokeArgs(null, [$a, $b]);
+
+        // It's empty because the first array contains all the contents of the second.
+        $expected = [];
+        $this->assertEquals($expected, $result);
+    }
 }

lib/tests/session_redis_test.php

@@ -94,6 +94,7 @@ class core_session_redis_testcase extends advanced_testcase {
     public function test_normal_session_start_stop_works() {
         $sess = new \core\session\redis();
         $sess->init();
+        $sess->set_requires_write_lock(true);
         $this->assertTrue($sess->handler_open('Not used', 'Not used'));
         $this->assertSame('', $sess->handler_read('sess1'));
         $this->assertTrue($sess->handler_write('sess1', 'DATA'));
@@ -110,6 +111,7 @@ class core_session_redis_testcase extends advanced_testcase {
     public function test_session_blocks_with_existing_session() {
         $sess = new \core\session\redis();
         $sess->init();
+        $sess->set_requires_write_lock(true);
         $this->assertTrue($sess->handler_open('Not used', 'Not used'));
         $this->assertSame('', $sess->handler_read('sess1'));
         $this->assertTrue($sess->handler_write('sess1', 'DATA'));
@@ -121,6 +123,7 @@ class core_session_redis_testcase extends advanced_testcase {
 
         $sessblocked = new \core\session\redis();
         $sessblocked->init();
+        $sessblocked->set_requires_write_lock(true);
         $this->assertTrue($sessblocked->handler_open('Not used', 'Not used'));
 
         // Trap the error log and send it to stdOut so we can expect output at the right times.
@@ -143,6 +146,7 @@ class core_session_redis_testcase extends advanced_testcase {
     public function test_session_is_destroyed_when_it_does_not_exist() {
         $sess = new \core\session\redis();
         $sess->init();
+        $sess->set_requires_write_lock(true);
         $this->assertTrue($sess->handler_open('Not used', 'Not used'));
         $this->assertTrue($sess->handler_destroy('sess-destroy'));
         $this->assertSessionNoLocks();
@@ -151,6 +155,7 @@ class core_session_redis_testcase extends advanced_testcase {
     public function test_session_is_destroyed_when_we_have_it_open() {
         $sess = new \core\session\redis();
         $sess->init();
+        $sess->set_requires_write_lock(true);
         $this->assertTrue($sess->handler_open('Not used', 'Not used'));
         $this->assertSame('', $sess->handler_read('sess-destroy'));
         $this->assertTrue($sess->handler_destroy('sess-destroy'));
@@ -160,8 +165,10 @@ class core_session_redis_testcase extends advanced_testcase {
 
     public function test_multiple_sessions_do_not_interfere_with_each_other() {
         $sess1 = new \core\session\redis();
+        $sess1->set_requires_write_lock(true);
         $sess1->init();
         $sess2 = new \core\session\redis();
+        $sess2->set_requires_write_lock(true);
         $sess2->init();
 
         // Initialize session 1.
@@ -203,6 +210,7 @@ class core_session_redis_testcase extends advanced_testcase {
     public function test_multiple_sessions_work_with_a_single_instance() {
         $sess = new \core\session\redis();
         $sess->init();
+        $sess->set_requires_write_lock(true);
 
         // Initialize session 1.
         $this->assertTrue($sess->handler_open('Not used', 'Not used'));
@@ -223,6 +231,7 @@ class core_session_redis_testcase extends advanced_testcase {
     public function test_session_exists_returns_valid_values() {
         $sess = new \core\session\redis();
         $sess->init();
+        $sess->set_requires_write_lock(true);
 
         $this->assertTrue($sess->handler_open('Not used', 'Not used'));
         $this->assertSame('', $sess->handler_read('sess1'));

lib/upgrade.txt

@@ -38,6 +38,10 @@ information provided here is intended especially for developers.
 * H5P libraries have been moved from /lib/h5p to h5p/h5plib as an h5plib plugintype.
 * mdn-polyfills has been renamed to polyfills. The reason there is no polyfill from the MDN is
   because there is no example polyfills on the MDN for this functionality.
+* AJAX pages can be called without requiring a session lock if they set READ_ONLY_SESSION to true, eg.
+  define('READ_ONLY_SESSION', true); Note - this also requires $CFG->enable_read_only_sessions to be set to true.
+* External functions can be called without requiring a session lock if they define 'readonlysession' => true in
+  db/services.php. Note - this also requires $CFG->enable_read_only_sessions to be set to true.
 
 === 3.8 ===
 * Add CLI option to notify all cron tasks to stop: admin/cli/cron.php --stop

message/output/popup/db/services.php

@@ -41,5 +41,6 @@ $functions = array(
         'type' => 'read',
         'ajax' => true,
         'services' => array(MOODLE_OFFICIAL_MOBILE_SERVICE),
+        'readonlysession' => true,
     ),
 );