MDL-58018 core: finish dev to support open sessions without a lock

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,7 +26,7 @@
  */
 
 define('AJAX_SCRIPT', true);
-define('REQUIRE_SESSION_LOCK', false);
+define('READ_ONLY_SESSION', true);
 
 /** Include config */
 require_once(__DIR__ . '/../../config.php');

lib/ajax/service.php

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

lib/classes/session/handler.php

@@ -34,8 +34,8 @@ defined('MOODLE_INTERNAL') || die();
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 abstract class handler {
-    /** @var boolean $haslock does the session need and/or have a lock? */
+    /** @var boolean $requireswritelock does the session need and/or have a lock? */
-    protected $haslock = false;
+    protected $requireswritelock = false;
 
     /**
      * Start the session.
@@ -50,9 +50,9 @@ abstract class handler {
      * with a write lock, then we will abort the session instead if able.
      */
     public function write_close() {
-        if ($this->has_writelock()) {
+        if ($this->requires_write_lock()) {
             session_write_close();
-            $this->haslock = false;
+            $this->requireswritelock = false;
         } else {
             $this->abort();
         }
@@ -65,7 +65,7 @@ abstract class handler {
      */
     public function abort() {
         session_abort();
-        $this->haslock = false;
+        $this->requireswritelock = false;
     }
 
     /**
@@ -73,10 +73,10 @@ abstract class handler {
      * 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 $needslock true if needs to be open for writing
+     * @param bool $requireswritelock true if needs to be open for writing
      */
-    public function set_needslock($needslock) {
+    public function set_requires_write_lock($requireswritelock) {
-        $this->haslock = $needslock;
+        $this->requireswritelock = $requireswritelock;
     }
 
     /**
@@ -84,8 +84,8 @@ abstract class handler {
      * start() if you support read-only sessions.
      * @return bool true if session is intended to have a write lock.
      */
-    public function has_writelock() {
+    public function requires_write_lock() {
-        return $this->haslock;
+        return $this->requireswritelock;
     }
 
     /**

lib/classes/session/manager.php

@@ -57,6 +57,9 @@ 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.
@@ -68,13 +71,9 @@ class manager {
      * as practical across environments.
      */
     public static function restart_with_write_lock() {
-        if (self::$sessionactive) {
+        if (self::$sessionactive && !self::$handler->requires_write_lock()) {
-            if (!self::$handler->has_writelock()) {
+            @self::$handler->abort();
-                @self::$handler->abort();
+            self::$sessionactive = false;
-                self::$sessionactive = false;
-            }
-        }
-        if (!self::$sessionactive) {
             self::start_session(true);
         }
     }
@@ -104,49 +103,34 @@ class manager {
             return;
         }
 
-        if (defined('REQUIRE_SESSION_LOCK') && defined('ENABLE_READ_ONLY_SESSIONS') && ENABLE_READ_ONLY_SESSIONS) {
+        if (defined('READ_ONLY_SESSION') && !empty($CFG->enable_read_only_sessions)) {
-            $needslock = REQUIRE_SESSION_LOCK;
+            $requireslock = !READ_ONLY_SESSION;
         } else {
-            $needslock = true; // For backwards compatibility, we default to assuming that a lock is needed.
+            $requireslock = true; // For backwards compatibility, we default to assuming that a lock is needed.
         }
-        self::start_session($needslock);
+        self::start_session($requireslock);
     }
 
     /**
      * Handles starting a session.
      *
-     * @param bool $needslock If this is false then no write lock will be acquired,
+     * @param bool $requireslock If this is false then no write lock will be acquired,
-     *                        and the session will be read-only.
+     *                           and the session will be read-only.
      */
-    private static function start_session(bool $needslock) {
+    private static function start_session(bool $requireslock) {
         global $PERF;
 
         try {
             self::$handler->init();
-            self::$handler->set_needslock($needslock);
+            self::$handler->set_requires_write_lock($requireslock);
             self::prepare_cookies();
             $isnewsession = empty($_COOKIE[session_name()]);
 
-            if (defined('DEBUG_SESSION_TIMING_MIN_TIME')) {
-                $starttime = microtime(true);
-            }
-
             if (!self::$handler->start()) {
                 // Could not successfully start/recover session.
                 throw new \core\session\exception(get_string('servererror'));
             }
 
-            // DEBUG_SESSION_TIMING_MIN_TIME if defined is a float, and we'll show a message for
-            // session aquisition that exceeds this amount in seconds.
-            if (defined('DEBUG_SESSION_TIMING_MIN_TIME')) {
-                $duration = microtime(true) - $starttime;
-                if ($duration > DEBUG_SESSION_TIMING_MIN_TIME) {
-                    /* don't log the raw session ID, since the session ID itself is a secret */
-                    error_log("slow_session_id:".hash('sha256', session_id())."|duration:".
-                        round($duration, 3)."s|session_url:".$_SERVER['PHP_SELF']."|write_lock:".($needslock ? '1' : '0'));
-                }
-            }
-
             // Grab the time when session lock starts.
             $PERF->sessionlock['gained'] = microtime(true);
             $PERF->sessionlock['wait'] = $PERF->sessionlock['gained'] - $PERF->sessionlock['start'];
@@ -154,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.
@@ -708,6 +696,28 @@ 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);
+                    }
+                }
+            }
         }
 
         // More control over whether session data
@@ -717,9 +727,13 @@ class manager {
             // indication session start was clean.
             self::$handler->write_close();
         } else {
-            // Otherwise, if possibile lock exists want
+            // Otherwise, if possible lock exists want
             // to clear it, but do not write session.
-            @self::$handler->abort();
+            // If the $handler has not been set then
+            // there is no session to abort.
+            if (isset(self::$handler)) {
+                @self::$handler->abort();
+            }
         }
 
         self::$sessionactive = false;
@@ -1363,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,7 +101,7 @@ class memcached extends handler {
      * @return bool success
      */
     public function start() {
-        ini_set('memcached.sess_locking', $this->has_writelock() ? '1' : '0');
+        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

lib/db/services.php

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

lib/externallib.php

@@ -161,10 +161,10 @@ class external_api {
             } else {
                 $function->loginrequired = true;
             }
-            if (isset($functions[$function->name]['requiresessionlock'])) {
+            if (isset($functions[$function->name]['readonlysession'])) {
-                $function->requiresessionlock = $functions[$function->name]['requiresessionlock'];
+                $function->readonlysession = $functions[$function->name]['readonlysession'];
             } else {
-                $function->requiresessionlock = true;
+                $function->readonlysession = false;
             }
         }
 
@@ -190,7 +190,8 @@ class external_api {
         $externalfunctioninfo = static::external_function_info($function);
 
         // Eventually this should shift into the various handlers and not be handled via config.
-        if ($externalfunctioninfo->requiresessionlock) {
+        $readonlysession = $externalfunctioninfo->readonlysession ?? false;
+        if (!$readonlysession || empty($CFG->enable_read_only_sessions)) {
             \core\session\manager::restart_with_write_lock();
         }
 

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/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,6 +41,6 @@ $functions = array(
         'type' => 'read',
         'ajax' => true,
         'services' => array(MOODLE_OFFICIAL_MOBILE_SERVICE),
-        'requiresessionlock' => false,
+        'readonlysession' => true,
     ),
 );

pluginfile.php

@@ -28,7 +28,6 @@
 if (!defined('NO_DEBUG_DISPLAY')) {
     define('NO_DEBUG_DISPLAY', true);
 }
-define('REQUIRE_SESSION_LOCK', false); // Plugins will need to acquire a lock if they need one.
 
 require_once('config.php');
 require_once('lib/filelib.php');