From 2a0ae4a9b58a4ce529c55612edee191bff7623e4 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sat, 1 Oct 2022 11:36:02 +0900 Subject: [PATCH 01/12] fix: the default values for Redis Make the same as Cache Redis config. See https://github.com/codeigniter4/CodeIgniter4/blob/ed0cdd00eaf20f7d6efb373af991111ead4fece8/app/Config/Cache.php#L155-L161 --- system/Session/Handlers/RedisHandler.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index 93a601eade70..a72ca2525fbc 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -75,10 +75,10 @@ public function __construct(AppConfig $config, string $ipAddress) $this->savePath = [ 'host' => $matches[1], - 'port' => empty($matches[2]) ? null : $matches[2], + 'port' => empty($matches[2]) ? 6379 : $matches[2], 'password' => preg_match('#auth=([^\s&]+)#', $matches[3], $match) ? $match[1] : null, - 'database' => preg_match('#database=(\d+)#', $matches[3], $match) ? (int) $match[1] : null, - 'timeout' => preg_match('#timeout=(\d+\.\d+)#', $matches[3], $match) ? (float) $match[1] : null, + 'database' => preg_match('#database=(\d+)#', $matches[3], $match) ? (int) $match[1] : 0, + 'timeout' => preg_match('#timeout=(\d+\.\d+)#', $matches[3], $match) ? (float) $match[1] : 0, ]; preg_match('#prefix=([^\s&]+)#', $matches[3], $match) && $this->keyPrefix = $match[1]; From c9e20428a786aee0b6a044cdad46a3adf6064274 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sat, 1 Oct 2022 17:00:01 +0900 Subject: [PATCH 02/12] test: add RedisHandlerTest --- .../Handlers/Database/RedisHandlerTest.php | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 tests/system/Session/Handlers/Database/RedisHandlerTest.php diff --git a/tests/system/Session/Handlers/Database/RedisHandlerTest.php b/tests/system/Session/Handlers/Database/RedisHandlerTest.php new file mode 100644 index 000000000000..844ed504c49f --- /dev/null +++ b/tests/system/Session/Handlers/Database/RedisHandlerTest.php @@ -0,0 +1,114 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Session\Handlers\Database; + +use CodeIgniter\Session\Handlers\RedisHandler; +use CodeIgniter\Test\CIUnitTestCase; +use Config\App as AppConfig; +use Redis; + +/** + * @internal + */ +final class RedisHandlerTest extends CIUnitTestCase +{ + private string $sessionName = 'ci_session'; + private string $sessionSavePath = 'tcp://127.0.0.1:6379'; + private string $userIpAddress = '127.0.0.1'; + + protected function setUp(): void + { + parent::setUp(); + + if (! class_exists(Redis::class)) { + $this->markTestSkipped( + 'Redis Session Handler requires PHP Redis extention and Redis server' + ); + } + } + + private function getInstance($options = []) + { + $defaults = [ + 'sessionDriver' => RedisHandler::class, + 'sessionCookieName' => $this->sessionName, + 'sessionExpiration' => 7200, + 'sessionSavePath' => $this->sessionSavePath, + 'sessionMatchIP' => false, + 'sessionTimeToUpdate' => 300, + 'sessionRegenerateDestroy' => false, + 'cookieDomain' => '', + 'cookiePrefix' => '', + 'cookiePath' => '/', + 'cookieSecure' => false, + 'cookieSameSite' => 'Lax', + ]; + + $config = array_merge($defaults, $options); + $appConfig = new AppConfig(); + + foreach ($config as $key => $c) { + $appConfig->{$key} = $c; + } + + return new RedisHandler($appConfig, $this->userIpAddress); + } + + public function testOpen() + { + $handler = $this->getInstance(); + $this->assertTrue($handler->open($this->sessionSavePath, $this->sessionName)); + } + + public function testWrite() + { + $handler = $this->getInstance(); + $handler->open($this->sessionSavePath, $this->sessionName); + $handler->read('555556b43phsnnf8if6bo33b635e4447'); + + $data = <<<'DATA' + __ci_last_regenerate|i:1664607454;_ci_previous_url|s:32:"http://localhost:8080/index.php/";key|s:5:"value"; + DATA; + $this->assertTrue($handler->write('555556b43phsnnf8if6bo33b635e4447', $data)); + + $handler->close(); + } + + public function testReadSuccess() + { + $handler = $this->getInstance(); + $handler->open($this->sessionSavePath, $this->sessionName); + + $expected = <<<'DATA' + __ci_last_regenerate|i:1664607454;_ci_previous_url|s:32:"http://localhost:8080/index.php/";key|s:5:"value"; + DATA; + $this->assertSame($expected, $handler->read('555556b43phsnnf8if6bo33b635e4447')); + + $handler->close(); + } + + public function testReadFailure() + { + $handler = $this->getInstance(); + $handler->open($this->sessionSavePath, $this->sessionName); + + $this->assertSame('', $handler->read('123456b43phsnnf8if6bo33b635e4321')); + + $handler->close(); + } + + public function testGC() + { + $handler = $this->getInstance(); + $this->assertSame(1, $handler->gc(3600)); + } +} From 4aeba6fea209e0fa4758e388f35f1a227d262d01 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sat, 1 Oct 2022 17:01:01 +0900 Subject: [PATCH 03/12] refactor: reuse variable --- system/Session/Handlers/RedisHandler.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index a72ca2525fbc..bd829ad55411 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -266,14 +266,15 @@ public function gc($max_lifetime) */ protected function lockSession(string $sessionID): bool { + $lockKey = $this->keyPrefix . $sessionID . ':lock'; + // PHP 7 reuses the SessionHandler object on regeneration, // so we need to check here if the lock key is for the // correct session ID. - if ($this->lockKey === $this->keyPrefix . $sessionID . ':lock') { + if ($this->lockKey === $lockKey) { return $this->redis->expire($this->lockKey, 300); } - $lockKey = $this->keyPrefix . $sessionID . ':lock'; $attempt = 0; do { From 1749d9ba340af1bb6494c2d0df5dff637964cc43 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sat, 1 Oct 2022 17:02:05 +0900 Subject: [PATCH 04/12] docs: add @param --- system/Session/Handlers/RedisHandler.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index bd829ad55411..580a0799a148 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -58,6 +58,8 @@ class RedisHandler extends BaseHandler protected $sessionExpiration = 7200; /** + * @param string $ipAddress User's IP address + * * @throws SessionException */ public function __construct(AppConfig $config, string $ipAddress) From b574d5dce0ad74f939460ccb0713c6550eaa851f Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 5 Oct 2022 11:26:15 +0900 Subject: [PATCH 05/12] refactor: use class constant for magic number --- system/Session/Handlers/RedisHandler.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index 580a0799a148..1ca102773d82 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -22,6 +22,11 @@ */ class RedisHandler extends BaseHandler { + /** + * @var int + */ + private const REDIS_DEFAULT_PORT = 6379; + /** * phpRedis instance * @@ -77,7 +82,7 @@ public function __construct(AppConfig $config, string $ipAddress) $this->savePath = [ 'host' => $matches[1], - 'port' => empty($matches[2]) ? 6379 : $matches[2], + 'port' => empty($matches[2]) ? self::REDIS_DEFAULT_PORT : $matches[2], 'password' => preg_match('#auth=([^\s&]+)#', $matches[3], $match) ? $match[1] : null, 'database' => preg_match('#database=(\d+)#', $matches[3], $match) ? (int) $match[1] : 0, 'timeout' => preg_match('#timeout=(\d+\.\d+)#', $matches[3], $match) ? (float) $match[1] : 0, From 62e0b44f235faac8876d433d654b4e704bb6f9c3 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 5 Oct 2022 11:26:56 +0900 Subject: [PATCH 06/12] fix: support integer timeout --- system/Session/Handlers/RedisHandler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index 1ca102773d82..158b8ac2fcd1 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -85,7 +85,7 @@ public function __construct(AppConfig $config, string $ipAddress) 'port' => empty($matches[2]) ? self::REDIS_DEFAULT_PORT : $matches[2], 'password' => preg_match('#auth=([^\s&]+)#', $matches[3], $match) ? $match[1] : null, 'database' => preg_match('#database=(\d+)#', $matches[3], $match) ? (int) $match[1] : 0, - 'timeout' => preg_match('#timeout=(\d+\.\d+)#', $matches[3], $match) ? (float) $match[1] : 0, + 'timeout' => preg_match('#timeout=(\d+\.\d+|\d+)#', $matches[3], $match) ? (float) $match[1] : 0, ]; preg_match('#prefix=([^\s&]+)#', $matches[3], $match) && $this->keyPrefix = $match[1]; From 5cf69797f159e70998acd2a3f46a1066da7e18bb Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 5 Oct 2022 11:31:03 +0900 Subject: [PATCH 07/12] refactor: extract method --- system/Session/Handlers/RedisHandler.php | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index 158b8ac2fcd1..285986c3fdf2 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -71,6 +71,19 @@ public function __construct(AppConfig $config, string $ipAddress) { parent::__construct($config, $ipAddress); + $this->setSavePath(); + + if ($this->matchIP === true) { + $this->keyPrefix .= $this->ipAddress . ':'; + } + + $this->sessionExpiration = empty($config->sessionExpiration) + ? (int) ini_get('session.gc_maxlifetime') + : (int) $config->sessionExpiration; + } + + protected function setSavePath(): void + { if (empty($this->savePath)) { throw SessionException::forEmptySavepath(); } @@ -92,14 +105,6 @@ public function __construct(AppConfig $config, string $ipAddress) } else { throw SessionException::forInvalidSavePathFormat($this->savePath); } - - if ($this->matchIP === true) { - $this->keyPrefix .= $this->ipAddress . ':'; - } - - $this->sessionExpiration = empty($config->sessionExpiration) - ? (int) ini_get('session.gc_maxlifetime') - : (int) $config->sessionExpiration; } /** From 7d45205ddbe97a7508bd25091017b9c2ea7a7113 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 5 Oct 2022 11:41:51 +0900 Subject: [PATCH 08/12] test: add tests for savePath timeout --- .../Handlers/Database/RedisHandlerTest.php | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/system/Session/Handlers/Database/RedisHandlerTest.php b/tests/system/Session/Handlers/Database/RedisHandlerTest.php index 844ed504c49f..9bf826e66ca8 100644 --- a/tests/system/Session/Handlers/Database/RedisHandlerTest.php +++ b/tests/system/Session/Handlers/Database/RedisHandlerTest.php @@ -63,6 +63,28 @@ private function getInstance($options = []) return new RedisHandler($appConfig, $this->userIpAddress); } + public function testSavePathTimeoutFloat() + { + $handler = $this->getInstance( + ['sessionSavePath' => 'tcp://127.0.0.1:6379?timeout=2.5'] + ); + + $savePath = $this->getPrivateProperty($handler, 'savePath'); + + $this->assertSame(2.5, $savePath['timeout']); + } + + public function testSavePathTimeoutInt() + { + $handler = $this->getInstance( + ['sessionSavePath' => 'tcp://127.0.0.1:6379?timeout=10'] + ); + + $savePath = $this->getPrivateProperty($handler, 'savePath'); + + $this->assertSame(10.0, $savePath['timeout']); + } + public function testOpen() { $handler = $this->getInstance(); From 07eadd2331dcd1a3acb45d4a9db79f9e89ee6793 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 5 Oct 2022 11:43:20 +0900 Subject: [PATCH 09/12] test: use @requires --- .../Session/Handlers/Database/RedisHandlerTest.php | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/tests/system/Session/Handlers/Database/RedisHandlerTest.php b/tests/system/Session/Handlers/Database/RedisHandlerTest.php index 9bf826e66ca8..004eba2842eb 100644 --- a/tests/system/Session/Handlers/Database/RedisHandlerTest.php +++ b/tests/system/Session/Handlers/Database/RedisHandlerTest.php @@ -17,6 +17,8 @@ use Redis; /** + * @requires extension redis + * * @internal */ final class RedisHandlerTest extends CIUnitTestCase @@ -25,17 +27,6 @@ final class RedisHandlerTest extends CIUnitTestCase private string $sessionSavePath = 'tcp://127.0.0.1:6379'; private string $userIpAddress = '127.0.0.1'; - protected function setUp(): void - { - parent::setUp(); - - if (! class_exists(Redis::class)) { - $this->markTestSkipped( - 'Redis Session Handler requires PHP Redis extention and Redis server' - ); - } - } - private function getInstance($options = []) { $defaults = [ From d97d2173080ec661393d19798d2c6e26c60e2a95 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 5 Oct 2022 11:45:20 +0900 Subject: [PATCH 10/12] refactor: use 0.0 instead of 0 The timeout is float. --- system/Session/Handlers/RedisHandler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index 285986c3fdf2..4640c3b9b657 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -98,7 +98,7 @@ protected function setSavePath(): void 'port' => empty($matches[2]) ? self::REDIS_DEFAULT_PORT : $matches[2], 'password' => preg_match('#auth=([^\s&]+)#', $matches[3], $match) ? $match[1] : null, 'database' => preg_match('#database=(\d+)#', $matches[3], $match) ? (int) $match[1] : 0, - 'timeout' => preg_match('#timeout=(\d+\.\d+|\d+)#', $matches[3], $match) ? (float) $match[1] : 0, + 'timeout' => preg_match('#timeout=(\d+\.\d+|\d+)#', $matches[3], $match) ? (float) $match[1] : 0.0, ]; preg_match('#prefix=([^\s&]+)#', $matches[3], $match) && $this->keyPrefix = $match[1]; From 90adf465f726ced768eae484fc8f63ca8e4e2992 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 5 Oct 2022 11:56:53 +0900 Subject: [PATCH 11/12] docs: remove @var for class constant --- system/Session/Handlers/RedisHandler.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index 4640c3b9b657..f3d9fc1a46de 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -22,9 +22,6 @@ */ class RedisHandler extends BaseHandler { - /** - * @var int - */ private const REDIS_DEFAULT_PORT = 6379; /** From 2a6ff864b1909732021afd295f03c33afa9e666e Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 6 Oct 2022 05:46:25 +0900 Subject: [PATCH 12/12] refactor: change constant name --- system/Session/Handlers/RedisHandler.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index f3d9fc1a46de..1e996b5f30b9 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -22,7 +22,7 @@ */ class RedisHandler extends BaseHandler { - private const REDIS_DEFAULT_PORT = 6379; + private const DEFAULT_PORT = 6379; /** * phpRedis instance @@ -92,7 +92,7 @@ protected function setSavePath(): void $this->savePath = [ 'host' => $matches[1], - 'port' => empty($matches[2]) ? self::REDIS_DEFAULT_PORT : $matches[2], + 'port' => empty($matches[2]) ? self::DEFAULT_PORT : $matches[2], 'password' => preg_match('#auth=([^\s&]+)#', $matches[3], $match) ? $match[1] : null, 'database' => preg_match('#database=(\d+)#', $matches[3], $match) ? (int) $match[1] : 0, 'timeout' => preg_match('#timeout=(\d+\.\d+|\d+)#', $matches[3], $match) ? (float) $match[1] : 0.0,