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 54b7a5e362914..e787559c4c170 100644 --- a/lib/base.php +++ b/lib/base.php @@ -456,6 +456,7 @@ public static function initSession() { } $session->set('LAST_ACTIVITY', time()); + $session->close(); } /** diff --git a/lib/private/AppFramework/Middleware/SessionMiddleware.php b/lib/private/AppFramework/Middleware/SessionMiddleware.php index f3fd2c991739f..32ac2b17ae5b1 100644 --- a/lib/private/AppFramework/Middleware/SessionMiddleware.php +++ b/lib/private/AppFramework/Middleware/SessionMiddleware.php @@ -51,8 +51,8 @@ public function __construct(ControllerMethodReflector $reflector, */ 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 2e3bd46da5b4b..b01887e39e20f 100644 --- a/lib/private/Session/CryptoSessionData.php +++ b/lib/private/Session/CryptoSessionData.php @@ -97,8 +97,17 @@ protected function initializeSession() { * @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 @@ public function exists(string $key): bool { * @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 @@ public function clear() { $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 6e0c54c6fab4e..f192b20cc95e3 100644 --- a/lib/private/Session/Internal.php +++ b/lib/private/Session/Internal.php @@ -68,8 +68,11 @@ public function __construct(string $name) { * @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 @@ public function remove(string $key) { } public function clear() { + $this->reopen(); $this->invoke('session_unset'); $this->regenerateId(); $this->startSession(true); @@ -120,6 +124,7 @@ public function close() { * @return void */ public function regenerateId(bool $deleteOldSession = true, bool $updateToken = false) { + $this->reopen(); $oldId = null; if ($updateToken) { @@ -171,8 +176,14 @@ public function getId(): string { /** * @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 0afd370336609..b9b3dba54b734 100644 --- a/lib/private/Session/Memory.php +++ b/lib/private/Session/Memory.php @@ -53,7 +53,6 @@ public function __construct(string $name) { * @param integer $value */ public function set(string $key, $value) { - $this->validateSession(); $this->data[$key] = $value; } @@ -80,7 +79,6 @@ public function exists(string $key): bool { * @param string $key */ public function remove(string $key) { - $this->validateSession(); unset($this->data[$key]); } @@ -110,8 +108,10 @@ public function getId(): string { /** * 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 2709e09d4ca4b..12cd716ec3f92 100644 --- a/lib/public/ISession.php +++ b/lib/public/ISession.php @@ -83,6 +83,14 @@ public function remove(string $key); */ 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 025cf1105a01a..fe45223361faa 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -256,6 +256,17 @@ */ '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 e787559c4c170..c0aee6c528f3a 100644 --- a/lib/base.php +++ b/lib/base.php @@ -455,7 +455,9 @@ public static function initSession() { \OC::$server->getUserSession()->logout(); } - $session->set('LAST_ACTIVITY', time()); + if (!self::hasSessionRelaxedExpiry()) { + $session->set('LAST_ACTIVITY', time()); + } $session->close(); } @@ -466,6 +468,13 @@ private static function getSessionLifeTime() { 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 f192b20cc95e3..87dd5ed60145c 100644 --- a/lib/private/Session/Internal.php +++ b/lib/private/Session/Internal.php @@ -178,7 +178,7 @@ public function getId(): string { */ public function reopen(): bool { if ($this->sessionClosed) { - $this->startSession(); + $this->startSession(false, false); $this->sessionClosed = false; return true; } @@ -225,7 +225,11 @@ private function invoke(string $functionName, array $parameters = [], bool $sile } } - 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 c0aee6c528f3a..1fca124f07215 100644 --- a/lib/base.php +++ b/lib/base.php @@ -445,7 +445,9 @@ public static function initSession() { 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 @@ public static function init() { $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)) {