diff --git a/application/front/controller/visitor/LoginController.php b/application/front/controller/visitor/LoginController.php index 4b881535..1b40944c 100644 --- a/application/front/controller/visitor/LoginController.php +++ b/application/front/controller/visitor/LoginController.php @@ -82,12 +82,12 @@ public function login(Request $request, Response $response): Response return $this->index($request, $response); } - $this->container->loginManager->handleSuccessfulLogin($this->container->environment); - $cookiePath = $this->container->basePath . '/'; $expirationTime = $this->saveLongLastingSession($request, $cookiePath); $this->renewUserSession($cookiePath, $expirationTime); + $this->container->loginManager->handleSuccessfulLogin($this->container->environment); + // Force referer from given return URL $this->container->environment['HTTP_REFERER'] = $request->getParam('returnurl'); @@ -133,7 +133,7 @@ protected function saveLongLastingSession(Request $request, string $cookiePath): $this->container->cookieManager->setCookieParameter( CookieManager::STAY_SIGNED_IN, $this->container->loginManager->getStaySignedInToken(), - $expirationTime, + $expirationTime, // This expirationTime is currently never checked by Shaarli $cookiePath ); @@ -142,14 +142,16 @@ protected function saveLongLastingSession(Request $request, string $cookiePath): protected function renewUserSession(string $cookiePath, int $expirationTime): void { + // Quoting https://www.php.net/manual/en/features.session.security.management.php + // "Session IDs must be regenerated when user privileges are elevated, + // such as after authenticating. session_regenerate_id() + // must be called prior to setting the authentication information to $_SESSION." + $this->container->sessionManager->regenerateId(true); // Send cookie with the new expiration date to the browser - $this->container->sessionManager->destroy(); $this->container->sessionManager->cookieParameters( $expirationTime, $cookiePath, $this->container->environment['SERVER_NAME'] ); - $this->container->sessionManager->start(); - $this->container->sessionManager->regenerateId(true); } } diff --git a/application/security/LoginManager.php b/application/security/LoginManager.php index b795b80e..31f70170 100644 --- a/application/security/LoginManager.php +++ b/application/security/LoginManager.php @@ -106,7 +106,7 @@ public function checkLoginState($clientIpId) if ($this->staySignedInToken === $this->cookieManager->getCookieParameter(CookieManager::STAY_SIGNED_IN)) { // The user client has a valid stay-signed-in cookie // Session information is updated with the current client information - $this->sessionManager->storeLoginInfo($clientIpId); + $this->sessionManager->storeSessionInfo($clientIpId); } elseif ( $this->sessionManager->hasSessionExpired() || $this->sessionManager->hasClientIpChanged($clientIpId) @@ -154,8 +154,8 @@ public function checkCredentials($clientIpId, $login, $password) || (true === $useLdapLogin && $this->checkCredentialsFromLdap($login, $password)) ) ) { - $this->sessionManager->storeLoginInfo($clientIpId); - $this->logger->info(format_log('Login successful', $clientIpId)); + $this->sessionManager->storeSessionInfo($clientIpId); + $this->logger->info(format_log('Login successful for user ' . $login, $clientIpId)); return true; } diff --git a/application/security/SessionManager.php b/application/security/SessionManager.php index ca186626..770cb932 100644 --- a/application/security/SessionManager.php +++ b/application/security/SessionManager.php @@ -2,6 +2,7 @@ namespace Shaarli\Security; +use Psr\Log\LoggerInterface; use Shaarli\Config\ConfigManager; /** @@ -35,6 +36,9 @@ class SessionManager /** @var string */ protected $savePath; + /** @var LoggerInterface */ + protected $logger; + /** * Constructor * @@ -42,11 +46,12 @@ class SessionManager * @param ConfigManager $conf ConfigManager instance * @param string $savePath Session save path returned by builtin function session_save_path() */ - public function __construct(&$session, $conf, string $savePath) + public function __construct($conf, $logger, &$session, string $savePath) { $this->session = &$session; $this->conf = $conf; $this->savePath = $savePath; + $this->logger = $logger; } /** @@ -61,6 +66,10 @@ public function initialize(): void if (!isset($this->session['LINKS_PER_PAGE'])) { $this->session['LINKS_PER_PAGE'] = $this->conf->get('general.links_per_page', 20); } + if (!empty($this->session['expires_on']) && $this->session['expires_on'] >= (time() + self::$SHORT_TIMEOUT)) { + // We deduce from the session-stored 'expires_on' value if we are currently in a staySignedIn state: + $this->setStaySignedIn(true); + } } /** @@ -135,11 +144,11 @@ public static function checkId($sessionId) } /** - * Store user login information after a successful login + * Store user login information on every page load when logged-in * * @param string $clientIpId Client IP address identifier */ - public function storeLoginInfo($clientIpId) + public function storeSessionInfo($clientIpId) { $this->session['ip'] = $clientIpId; $this->session['username'] = $this->conf->get('credentials.login'); @@ -164,15 +173,26 @@ public function extendSession() * * @return int New session expiration time */ - protected function extendTimeValidityBy($duration) + protected function extendTimeValidityBy($durationSecs) { - $expirationTime = time() + $duration; + $phpSessionLifetimeMin = $this->getPhpSessionLifetimeMin(); + if ($durationSecs > $phpSessionLifetimeMin * 60) { + $this->logger->warning("PHP session lifetime (session.gc_maxlifetime=" . $phpSessionLifetimeMin . "min)" + . " is lower than Shaarli session duration (" . $durationSecs / 60 . "min)"); + } + $expirationTime = time() + $durationSecs; $this->session['expires_on'] = $expirationTime; return $expirationTime; } + protected function getPhpSessionLifetimeMin() + { + return intval(ini_get("session.gc_maxlifetime")); + } + /** * Logout a user by unsetting all login information + * Currently called on every page if user is not logged-in! * * See: * - https://secure.php.net/manual/en/function.setcookie.php @@ -197,6 +217,7 @@ public function logout() public function hasSessionExpired() { if (empty($this->session['expires_on'])) { + // This is the case if the visitor is simply not logged in return true; } if (time() >= $this->session['expires_on']) { @@ -306,4 +327,12 @@ public function regenerateId(bool $deleteOldSession = false): bool { return session_regenerate_id($deleteOldSession); } + + /* + * Useful for debugging, to get the current state + */ + public function getStaySignedIn(): bool + { + return $this->staySignedIn; + } } diff --git a/doc/md/Server-configuration.md b/doc/md/Server-configuration.md index d2426a3a..0cd9e9ca 100644 --- a/doc/md/Server-configuration.md +++ b/doc/md/Server-configuration.md @@ -74,6 +74,14 @@ Some [plugins](Plugins.md) may require additional configuration. - [PHP 5 Changelog](https://www.php.net/ChangeLog-5.php) - [PHP: Bugs](https://bugs.php.net/) +#### Configuring session max duration + +Shaarli relies on PHP native [session handling](https://www.php.net/manual/en/book.session.php) based on `$SESSION`. A base Shaarli session lasts 1 hour. + +Due to this, the session duration is limited by the value of [`session.gc_maxlifetime`](https://www.php.net/manual/en/session.configuration.php#ini.session.gc-maxlifetime) (unit: minutes) set in your PHP configuration (_e.g._ `php.ini`). Its default value is `24min`. + +If you want to benefit from Shaarli _stay-signed-in_ feature that only terminates your session after one year of inactivity, you should set `session.gc_maxlifetime` to `525600` (1 year). + ## SSL/TLS (HTTPS) diff --git a/index.php b/index.php index cd517045..8c948358 100644 --- a/index.php +++ b/index.php @@ -63,7 +63,7 @@ !$conf->get('dev.debug') ? LogLevel::INFO : LogLevel::DEBUG, ['filename' => basename($conf->get('resource.log'))] ); -$sessionManager = new SessionManager($_SESSION, $conf, session_save_path()); +$sessionManager = new SessionManager($conf, $logger, $_SESSION, session_save_path()); $sessionManager->initialize(); $cookieManager = new CookieManager($_COOKIE); $banManager = new BanManager( diff --git a/tests/front/controller/visitor/LoginControllerTest.php b/tests/front/controller/visitor/LoginControllerTest.php index 00d9eab3..a5b8341d 100644 --- a/tests/front/controller/visitor/LoginControllerTest.php +++ b/tests/front/controller/visitor/LoginControllerTest.php @@ -201,14 +201,11 @@ public function testProcessLoginWithValidParameters(): void $this->container->loginManager->method('getStaySignedInToken')->willReturn(bin2hex(random_bytes(8))); $this->container->sessionManager->expects(static::never())->method('extendSession'); - $this->container->sessionManager->expects(static::once())->method('destroy'); + $this->container->sessionManager->expects(static::once())->method('regenerateId')->with(true); $this->container->sessionManager ->expects(static::once()) ->method('cookieParameters') - ->with(0, '/subfolder/', 'shaarli') - ; - $this->container->sessionManager->expects(static::once())->method('start'); - $this->container->sessionManager->expects(static::once())->method('regenerateId')->with(true); + ->with(0, '/subfolder/', 'shaarli'); $result = $this->controller->login($request, $response); @@ -268,14 +265,11 @@ public function testProcessLoginLongLastingSession(): void $this->container->loginManager->expects(static::once())->method('checkCredentials')->willReturn(true); $this->container->loginManager->method('getStaySignedInToken')->willReturn(bin2hex(random_bytes(8))); - $this->container->sessionManager->expects(static::once())->method('destroy'); + $this->container->sessionManager->expects(static::once())->method('regenerateId')->with(true); $this->container->sessionManager ->expects(static::once()) ->method('cookieParameters') - ->with(42, '/subfolder/', 'shaarli') - ; - $this->container->sessionManager->expects(static::once())->method('start'); - $this->container->sessionManager->expects(static::once())->method('regenerateId')->with(true); + ->with(42, '/subfolder/', 'shaarli'); $this->container->sessionManager->expects(static::once())->method('extendSession')->willReturn(42); $this->container->cookieManager = $this->createMock(CookieManager::class); diff --git a/tests/security/LoginManagerTest.php b/tests/security/LoginManagerTest.php index 18326d2c..f8776745 100644 --- a/tests/security/LoginManagerTest.php +++ b/tests/security/LoginManagerTest.php @@ -95,14 +95,15 @@ protected function setUp(): void $this->cookieManager->method('getCookieParameter')->willReturnCallback(function (string $key) { return $this->cookie[$key] ?? null; }); - $this->sessionManager = new SessionManager($this->session, $this->configManager, 'session_path'); + $mockLogger = $this->createMock(LoggerInterface::class); + $this->sessionManager = new SessionManager($this->configManager, $mockLogger, $this->session, 'session_path'); $this->banManager = $this->createMock(BanManager::class); $this->loginManager = new LoginManager( $this->configManager, $this->sessionManager, $this->cookieManager, $this->banManager, - $this->createMock(LoggerInterface::class) + $mockLogger ); $this->server['REMOTE_ADDR'] = $this->ipAddr; } diff --git a/tests/security/SessionManagerTest.php b/tests/security/SessionManagerTest.php index 7cc6e1a4..ff3d1105 100644 --- a/tests/security/SessionManagerTest.php +++ b/tests/security/SessionManagerTest.php @@ -2,6 +2,7 @@ namespace Shaarli\Security; +use Psr\Log\LoggerInterface; use Shaarli\FakeConfigManager; use Shaarli\TestCase; use Shaarli\Tests\Utils\ReferenceSessionIdHashes; @@ -41,8 +42,9 @@ protected function setUp(): void 'credentials.salt' => 'salt', 'security.session_protection_disabled' => false, ]); + $mockLogger = $this->createMock(LoggerInterface::class); $this->session = []; - $this->sessionManager = new SessionManager($this->session, $this->conf, 'session_path'); + $this->sessionManager = new SessionManager($this->conf, $mockLogger, $this->session, 'session_path'); } /** @@ -67,7 +69,8 @@ public function testCheckToken() $token => 1, ], ]; - $sessionManager = new SessionManager($session, $this->conf, 'session_path'); + $mockLogger = $this->createMock(LoggerInterface::class); + $sessionManager = new SessionManager($this->conf, $mockLogger, $session, 'session_path'); // check and destroy the token $this->assertTrue($sessionManager->checkToken($token)); @@ -157,9 +160,9 @@ public function testIsSessionIdInvalid() /** * Store login information after a successful login */ - public function testStoreLoginInfo() + public function testStoreSessionInfo() { - $this->sessionManager->storeLoginInfo('ip_id'); + $this->sessionManager->storeSessionInfo('ip_id'); $this->assertGreaterThan(time(), $this->session['expires_on']); $this->assertEquals('ip_id', $this->session['ip']);