From 9b4b72826ade5ab1bc7fb06048e62910ef607cd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 26 Apr 2022 12:57:58 +0200 Subject: [PATCH 1/3] Reopen sessions if we need to write to them instead of keeping them open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sessions are a locking operation until we write close them, so close them early and reopen later in case we want to write to them Signed-off-by: Julius Härtl --- lib/base.php | 1 + .../Middleware/SessionMiddleware.php | 4 ++-- lib/private/Session/CryptoSessionData.php | 17 +++++++++++++++++ lib/private/Session/Internal.php | 17 ++++++++++++++--- lib/private/Session/Memory.php | 6 +++--- lib/public/ISession.php | 8 ++++++++ 6 files changed, 45 insertions(+), 8 deletions(-) diff --git a/lib/base.php b/lib/base.php index 54b7a5e3629..e787559c4c1 100644 --- a/lib/base.php +++ b/lib/base.php @@ -456,6 +456,7 @@ class OC { } $session->set('LAST_ACTIVITY', time()); + $session->close(); } /** diff --git a/lib/private/AppFramework/Middleware/SessionMiddleware.php b/lib/private/AppFramework/Middleware/SessionMiddleware.php index f3fd2c99173..32ac2b17ae5 100644 --- a/lib/private/AppFramework/Middleware/SessionMiddleware.php +++ b/lib/private/AppFramework/Middleware/SessionMiddleware.php @@ -51,8 +51,8 @@ class SessionMiddleware extends Middleware { */ public function beforeController($controller, $methodName) { $useSession = $this->reflector->hasAnnotation('UseSession'); - if (!$useSession) { - $this->session->close(); + if ($useSession) { + $this->session->reopen(); } } diff --git a/lib/private/Session/CryptoSessionData.php b/lib/private/Session/CryptoSessionData.php index 2e3bd46da5b..b01887e39e2 100644 --- a/lib/private/Session/CryptoSessionData.php +++ b/lib/private/Session/CryptoSessionData.php @@ -97,8 +97,17 @@ class CryptoSessionData implements \ArrayAccess, ISession { * @param mixed $value */ public function set(string $key, $value) { + if ($this->get($key) === $value) { + // Do not write the session if the value hasn't changed to avoid reopening + return; + } + + $reopened = $this->reopen(); $this->sessionValues[$key] = $value; $this->isModified = true; + if ($reopened) { + $this->close(); + } } /** @@ -131,9 +140,13 @@ class CryptoSessionData implements \ArrayAccess, ISession { * @param string $key */ public function remove(string $key) { + $reopened = $this->reopen(); $this->isModified = true; unset($this->sessionValues[$key]); $this->session->remove(self::encryptedSessionName); + if ($reopened) { + $this->close(); + } } /** @@ -149,6 +162,10 @@ class CryptoSessionData implements \ArrayAccess, ISession { $this->session->clear(); } + public function reopen(): bool { + return $this->session->reopen(); + } + /** * Wrapper around session_regenerate_id * diff --git a/lib/private/Session/Internal.php b/lib/private/Session/Internal.php index 6e0c54c6fab..f192b20cc95 100644 --- a/lib/private/Session/Internal.php +++ b/lib/private/Session/Internal.php @@ -68,8 +68,11 @@ class Internal extends Session { * @param integer $value */ public function set(string $key, $value) { - $this->validateSession(); + $reopened = $this->reopen(); $_SESSION[$key] = $value; + if ($reopened) { + $this->close(); + } } /** @@ -101,6 +104,7 @@ class Internal extends Session { } public function clear() { + $this->reopen(); $this->invoke('session_unset'); $this->regenerateId(); $this->startSession(true); @@ -120,6 +124,7 @@ class Internal extends Session { * @return void */ public function regenerateId(bool $deleteOldSession = true, bool $updateToken = false) { + $this->reopen(); $oldId = null; if ($updateToken) { @@ -171,8 +176,14 @@ class Internal extends Session { /** * @throws \Exception */ - public function reopen() { - throw new \Exception('The session cannot be reopened - reopen() is only to be used in unit testing.'); + public function reopen(): bool { + if ($this->sessionClosed) { + $this->startSession(); + $this->sessionClosed = false; + return true; + } + + return false; } /** diff --git a/lib/private/Session/Memory.php b/lib/private/Session/Memory.php index 0afd3703366..b9b3dba54b7 100644 --- a/lib/private/Session/Memory.php +++ b/lib/private/Session/Memory.php @@ -53,7 +53,6 @@ class Memory extends Session { * @param integer $value */ public function set(string $key, $value) { - $this->validateSession(); $this->data[$key] = $value; } @@ -80,7 +79,6 @@ class Memory extends Session { * @param string $key */ public function remove(string $key) { - $this->validateSession(); unset($this->data[$key]); } @@ -110,8 +108,10 @@ class Memory extends Session { /** * Helper function for PHPUnit execution - don't use in non-test code */ - public function reopen() { + public function reopen(): bool { + $reopened = $this->sessionClosed; $this->sessionClosed = false; + return $reopened; } /** diff --git a/lib/public/ISession.php b/lib/public/ISession.php index 2709e09d4ca..12cd716ec3f 100644 --- a/lib/public/ISession.php +++ b/lib/public/ISession.php @@ -83,6 +83,14 @@ interface ISession { */ public function clear(); + /** + * Reopen a session for writing again + * + * @return bool true if the session was actually reopened, otherwise false + * @since 25.0.0 + */ + public function reopen(): bool; + /** * Close the session and release the lock * @since 7.0.0 From 9e1d4312555ddc1009450b1f6b7078ae35790593 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 16 Aug 2022 10:09:14 +0200 Subject: [PATCH 2/3] Add config option to disable strict session timeout to be able to use read_and_close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed https://github.com/nextcloud/server/issues/29356 Signed-off-by: Julius Härtl --- config/config.sample.php | 11 +++++++++++ lib/base.php | 11 ++++++++++- lib/private/Session/Internal.php | 10 +++++++--- 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/config/config.sample.php b/config/config.sample.php index 025cf1105a0..fe45223361f 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -256,6 +256,17 @@ $CONFIG = [ */ 'session_lifetime' => 60 * 60 * 24, +/** + * `true` enabled a relaxed session timeout, where the session timeout would no longer be + * handled by Nextcloud but by either the PHP garbage collection or the expiration of + * potential other session backends like redis. + * + * This may lead to sessions being available for longer than what session_lifetime uses but + * comes with performance benefits as sessions are no longer a locking operation for concurrent + * requests. + */ +'session_relaxed_expiry' => false, + /** * Enable or disable session keep-alive when a user is logged in to the Web UI. * Enabling this sends a "heartbeat" to the server to keep it from timing out. diff --git a/lib/base.php b/lib/base.php index e787559c4c1..c0aee6c528f 100644 --- a/lib/base.php +++ b/lib/base.php @@ -455,7 +455,9 @@ class OC { \OC::$server->getUserSession()->logout(); } - $session->set('LAST_ACTIVITY', time()); + if (!self::hasSessionRelaxedExpiry()) { + $session->set('LAST_ACTIVITY', time()); + } $session->close(); } @@ -466,6 +468,13 @@ class OC { return \OC::$server->getConfig()->getSystemValue('session_lifetime', 60 * 60 * 24); } + /** + * @return bool true if the session expiry should only be done by gc instead of an explicit timeout + */ + public static function hasSessionRelaxedExpiry(): bool { + return \OC::$server->getConfig()->getSystemValue('session_relaxed_expiry', false); + } + /** * Try to set some values to the required Nextcloud default */ diff --git a/lib/private/Session/Internal.php b/lib/private/Session/Internal.php index f192b20cc95..87dd5ed6014 100644 --- a/lib/private/Session/Internal.php +++ b/lib/private/Session/Internal.php @@ -178,7 +178,7 @@ class Internal extends Session { */ public function reopen(): bool { if ($this->sessionClosed) { - $this->startSession(); + $this->startSession(false, false); $this->sessionClosed = false; return true; } @@ -225,7 +225,11 @@ class Internal extends Session { } } - private function startSession(bool $silence = false) { - $this->invoke('session_start', [['cookie_samesite' => 'Lax']], $silence); + private function startSession(bool $silence = false, bool $readAndClose = true) { + $sessionParams = ['cookie_samesite' => 'Lax']; + if (\OC::hasSessionRelaxedExpiry()) { + $sessionParams['read_and_close'] = $readAndClose; + } + $this->invoke('session_start', [$sessionParams], $silence); } } From 1b43fbe06c7fa7eb16abe5509ecc952ddaf7c5be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Wed, 17 Aug 2022 12:10:03 +0200 Subject: [PATCH 3/3] Move setting of gc_maxlifetime to initSession MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/base.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/base.php b/lib/base.php index c0aee6c528f..1fca124f072 100644 --- a/lib/base.php +++ b/lib/base.php @@ -445,7 +445,9 @@ class OC { die(); } + //try to set the session lifetime $sessionLifeTime = self::getSessionLifeTime(); + @ini_set('gc_maxlifetime', (string)$sessionLifeTime); // session timeout if ($session->exists('LAST_ACTIVITY') && (time() - $session->get('LAST_ACTIVITY') > $sessionLifeTime)) { @@ -717,9 +719,6 @@ class OC { $config->deleteAppValue('core', 'cronErrors'); } } - //try to set the session lifetime - $sessionLifeTime = self::getSessionLifeTime(); - @ini_set('gc_maxlifetime', (string)$sessionLifeTime); // User and Groups if (!$systemConfig->getValue("installed", false)) {