From c6b5f18d0617b2d09aa097429f55eb6ae9f19bec Mon Sep 17 00:00:00 2001 From: Petr Skoda Date: Tue, 5 Aug 2014 12:11:43 +1200 Subject: [PATCH] MDL-46561 session: use full session validation in \core\session\manager::session_exists() --- auth/upgrade.txt | 5 +++++ lib/classes/session/database.php | 11 ++++----- lib/classes/session/file.php | 4 ++-- lib/classes/session/handler.php | 4 ++-- lib/classes/session/manager.php | 24 +++++++++++++++++++- lib/classes/session/memcache.php | 4 ++-- lib/classes/session/memcached.php | 4 ++-- lib/tests/session_manager_test.php | 36 +++++++++++++++++++++++++++++- 8 files changed, 75 insertions(+), 17 deletions(-) diff --git a/auth/upgrade.txt b/auth/upgrade.txt index 8334fed703d..5b511346168 100644 --- a/auth/upgrade.txt +++ b/auth/upgrade.txt @@ -1,6 +1,11 @@ This files describes API changes in /auth/* - plugins, information provided here is intended especially for developers. +=== 2.8 === + +* \core\session\manager::session_exists() now verifies the session is active + instead of only checking the session data is present in low level session handler + === 2.7 === * If you are returning a url in method change_password_url() from config, please make sure it is set before trying to use it. diff --git a/lib/classes/session/database.php b/lib/classes/session/database.php index dfa2e1ea504..ab19a781797 100644 --- a/lib/classes/session/database.php +++ b/lib/classes/session/database.php @@ -82,19 +82,16 @@ class database extends handler { } /** - * Check for existing session with id $sid. + * Check the backend contains data for this session id. * - * Note: this verifies the storage backend only, not the actual session records. + * Note: this is intended to be called from manager::session_exists() only. * * @param string $sid * @return bool true if session found. */ public function session_exists($sid) { - try { - return $this->database->record_exists('sessions', array('sid'=>$sid, 'state'=>0)); - } catch (\dml_exception $ex) { - return false; - } + // It was already checked in the calling code that the record in sessions table exists. + return true; } /** diff --git a/lib/classes/session/file.php b/lib/classes/session/file.php index 47a678c248c..b454e0e7b90 100644 --- a/lib/classes/session/file.php +++ b/lib/classes/session/file.php @@ -76,9 +76,9 @@ class file extends handler { } /** - * Check for existing session with id $sid. + * Check the backend contains data for this session id. * - * Note: this verifies the storage backend only, not the actual session records. + * Note: this is intended to be called from manager::session_exists() only. * * @param string $sid * @return bool true if session found. diff --git a/lib/classes/session/handler.php b/lib/classes/session/handler.php index 995784a4e8c..276f553ce45 100644 --- a/lib/classes/session/handler.php +++ b/lib/classes/session/handler.php @@ -48,9 +48,9 @@ abstract class handler { public abstract function init(); /** - * Check for existing session with id $sid. + * Check the backend contains data for this session id. * - * Note: this verifies the storage backend only, not the actual session records. + * Note: this is intended to be called from manager::session_exists() only. * * @param string $sid * @return bool true if session found. diff --git a/lib/classes/session/manager.php b/lib/classes/session/manager.php index e0d8bc00cfc..9e1378b0c08 100644 --- a/lib/classes/session/manager.php +++ b/lib/classes/session/manager.php @@ -524,12 +524,34 @@ class manager { /** * Does the PHP session with given id exist? * - * Note: this does not actually verify the presence of sessions record. + * The session must exist both in session table and actual + * session backend and the session must not be timed out. + * + * Timeout evaluation is simplified, the auth hooks are not executed. * * @param string $sid * @return bool */ public static function session_exists($sid) { + global $DB, $CFG; + + if (empty($CFG->version)) { + // Not installed yet, do not try to access database. + return false; + } + + // Note: add sessions->state checking here if it gets implemented. + if (!$record = $DB->get_record('sessions', array('sid' => $sid), 'id, userid, timemodified')) { + return false; + } + + if (empty($record->userid) or isguestuser($record->userid)) { + // Ignore guest and not-logged-in timeouts, there is very little risk here. + } else if ($record->timemodified < time() - $CFG->sessiontimeout) { + return false; + } + + // There is no need the existence of handler storage in public API. self::load_handler(); return self::$handler->session_exists($sid); } diff --git a/lib/classes/session/memcache.php b/lib/classes/session/memcache.php index fc3005064c0..734d8526d31 100644 --- a/lib/classes/session/memcache.php +++ b/lib/classes/session/memcache.php @@ -111,9 +111,9 @@ class memcache extends handler { } /** - * Checks for existing session with given id. + * Check the backend contains data for this session id. * - * Note: this verifies the storage backend only, not the actual session records. + * Note: this is intended to be called from manager::session_exists() only. * * @param string $sid PHP session ID * @return bool true if session found. diff --git a/lib/classes/session/memcached.php b/lib/classes/session/memcached.php index 73bf1bb343c..73392cb5ea8 100644 --- a/lib/classes/session/memcached.php +++ b/lib/classes/session/memcached.php @@ -125,9 +125,9 @@ class memcached extends handler { } /** - * Check for existing session with id $sid. + * Check the backend contains data for this session id. * - * Note: this verifies the storage backend only, not the actual session records. + * Note: this is intended to be called from manager::session_exists() only. * * @param string $sid * @return bool true if session found. diff --git a/lib/tests/session_manager_test.php b/lib/tests/session_manager_test.php index 5333917b9f3..b9db594c0c2 100644 --- a/lib/tests/session_manager_test.php +++ b/lib/tests/session_manager_test.php @@ -174,14 +174,48 @@ class core_session_manager_testcase extends advanced_testcase { } public function test_session_exists() { - global $CFG; + global $CFG, $DB; $this->resetAfterTest(); + $this->assertFalse(\core\session\manager::session_exists('abc')); + + $user = $this->getDataGenerator()->create_user(); + $guest = guest_user(); + // The file handler is used by default, so let's fake the data somehow. $sid = md5('hokus'); mkdir("$CFG->dataroot/sessions/", $CFG->directorypermissions, true); touch("$CFG->dataroot/sessions/sess_$sid"); + $this->assertFalse(\core\session\manager::session_exists($sid)); + + $record = new stdClass(); + $record->userid = 0; + $record->sid = $sid; + $record->timecreated = time(); + $record->timemodified = $record->timecreated; + $record->id = $DB->insert_record('sessions', $record); + + $this->assertTrue(\core\session\manager::session_exists($sid)); + + $record->timecreated = time() - $CFG->sessiontimeout - 100; + $record->timemodified = $record->timecreated + 10; + $DB->update_record('sessions', $record); + + $this->assertTrue(\core\session\manager::session_exists($sid)); + + $record->userid = $guest->id; + $DB->update_record('sessions', $record); + + $this->assertTrue(\core\session\manager::session_exists($sid)); + + $record->userid = $user->id; + $DB->update_record('sessions', $record); + + $this->assertFalse(\core\session\manager::session_exists($sid)); + + $CFG->sessiontimeout = $CFG->sessiontimeout + 3000; + $this->assertTrue(\core\session\manager::session_exists($sid)); }