From d5eaa5224e5a6afeb0cbe4abadd119ac12142ba7 Mon Sep 17 00:00:00 2001
From: Cameron Ball <cameronball@catalyst-au.net>
Date: Fri, 24 Sep 2021 14:09:34 +0800
Subject: [PATCH] MDL-72670 session: Correct read only debugging logic

Prior to this patch the debugging mode (when enabled) would trigger
on everywhere, regardless of whether or not READ_ONLY_SESSION is defined.

This patch modifies that behaviour so that the debugging only kicks in
if READ_ONLY_SESSION is defined and set to true.
---
 lib/classes/session/manager.php       | 35 ++++++++++++++++++++++++---
 lib/dml/tests/dml_read_slave_test.php |  2 +-
 lib/externallib.php                   |  2 +-
 3 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/lib/classes/session/manager.php b/lib/classes/session/manager.php
index 8a3f0a5487046..10f8a4960c39f 100644
--- a/lib/classes/session/manager.php
+++ b/lib/classes/session/manager.php
@@ -60,6 +60,15 @@ class manager {
     /** @var array Stores the the SESSION before a request is performed, used to check incorrect read-only modes */
     private static $priorsession = [];
 
+    /**
+     * @var bool Used to trigger the SESSION mutation warning without actually preventing SESSION mutation.
+     * This variable is used to "copy" what the $requireslock parameter  does in start_session().
+     * Once requireslock is set in start_session it's later accessible via $handler->requires_write_lock,
+     * When using $CFG->enable_read_only_sessions_debug mode, this variable serves the same purpose without
+     * actually setting the handler as requiring a write lock.
+     */
+    private static $requireslockdebug;
+
     /**
      * If the current session is not writeable, abort it, and re-open it
      * requesting (and blocking) until a write lock is acquired.
@@ -69,8 +78,16 @@ class manager {
      * 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.
+     *
+     * @param bool $readonlysession Used by debugging logic to determine if whatever
+     *                              triggered the restart (e.g., a webservice) declared
+     *                              itself as read only.
      */
-    public static function restart_with_write_lock() {
+    public static function restart_with_write_lock(bool $readonlysession) {
+        global $CFG;
+
+        self::$requireslockdebug = !$readonlysession;
+
         if (self::$sessionactive && !self::$handler->requires_write_lock()) {
             @self::$handler->abort();
             self::$sessionactive = false;
@@ -108,6 +125,17 @@ public static function start() {
         } else {
             $requireslock = true; // For backwards compatibility, we default to assuming that a lock is needed.
         }
+
+        // By default make the two variables the same. This means that when they are
+        // checked below in start_session and write_close there is no possibility for
+        // the debug version to "accidentally" execute the debug mode.
+        self::$requireslockdebug = $requireslock;
+        if (defined('READ_ONLY_SESSION') && !empty($CFG->enable_read_only_sessions_debug)) {
+            // Only change the debug variable if READ_ONLY_SESSION is declared,
+            // as would happen with the real requireslock variable.
+            self::$requireslockdebug = !READ_ONLY_SESSION;
+        }
+
         self::start_session($requireslock);
     }
 
@@ -138,9 +166,10 @@ private static function start_session(bool $requireslock) {
             self::$sessionactive = true; // Set here, so the session can be cleared if the security check fails.
             self::check_security();
 
-            if (!$requireslock || isset($CFG->enable_read_only_sessions_debug)) {
+            if (!$requireslock || !self::$requireslockdebug) {
                 self::$priorsession = (array) $_SESSION['SESSION'];
             }
+
             if (!empty($CFG->enable_read_only_sessions) && isset($_SESSION['SESSION']->cachestore_session)) {
                 $caches = join(', ', array_keys($_SESSION['SESSION']->cachestore_session));
                 $caches = str_replace('default_session-', '', $caches);
@@ -705,7 +734,7 @@ public static function write_close() {
             self::sessionlock_debugging();
 
             $requireslock = self::$handler->requires_write_lock();
-            if (!$requireslock || isset($CFG->enable_read_only_sessions_debug)) {
+            if (!$requireslock || !self::$requireslockdebug) {
                 // 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(
diff --git a/lib/dml/tests/dml_read_slave_test.php b/lib/dml/tests/dml_read_slave_test.php
index 58a6d68640c09..4ee6fc9c620aa 100644
--- a/lib/dml/tests/dml_read_slave_test.php
+++ b/lib/dml/tests/dml_read_slave_test.php
@@ -427,6 +427,6 @@ public function test_sessions() : void {
             $this->assertTrue($DB->perf_get_reads() > 0);
         });
 
-        \core\session\manager::restart_with_write_lock();
+        \core\session\manager::restart_with_write_lock(false);
     }
 }
diff --git a/lib/externallib.php b/lib/externallib.php
index da78def4ec655..a3ae65e4d6c47 100644
--- a/lib/externallib.php
+++ b/lib/externallib.php
@@ -198,7 +198,7 @@ public static function call_external_function($function, $args, $ajaxonly=false)
         // 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();
+            \core\session\manager::restart_with_write_lock($readonlysession);
         }
 
         $currentpage = $PAGE;