From a2101721d14fb0b281081571b45ec1374ebcc46a Mon Sep 17 00:00:00 2001 From: Robin Brabants Date: Wed, 30 Oct 2024 11:53:36 +0100 Subject: [PATCH 1/5] support authentication with non-default users for Redis clusters Signed-off-by: Robin Brabants --- psalm-baseline.xml | 1 + src/RedisClusterOptions.php | 30 +++++++++++++++++-- src/RedisClusterOptionsFromIni.php | 24 +++++++-------- src/RedisClusterResourceManager.php | 31 +++++++++----------- test/unit/RedisClusterOptionsFromIniTest.php | 6 ++-- test/unit/RedisClusterOptionsTest.php | 10 ++++--- 6 files changed, 64 insertions(+), 38 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 3f9ba3a..c579c18 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -52,6 +52,7 @@ + diff --git a/src/RedisClusterOptions.php b/src/RedisClusterOptions.php index f9c7567..5f4c0a7 100644 --- a/src/RedisClusterOptions.php +++ b/src/RedisClusterOptions.php @@ -64,7 +64,9 @@ final class RedisClusterOptions extends AdapterOptions /** @psalm-var array */ private array $libOptions = []; - private string $password = ''; + private ?string $username = null; + + private ?string $password = null; private ?SslContext $sslContext = null; @@ -248,7 +250,20 @@ public function getLibOption(int $option, mixed $default = null): mixed return $this->libOptions[$option] ?? $default; } - public function getPassword(): string + public function getUsername(): ?string + { + return $this->username; + } + + /** + * @psalm-param non-empty-string $username + */ + public function setUsername(string $username): void + { + $this->username = $username; + } + + public function getPassword(): ?string { return $this->password; } @@ -261,6 +276,17 @@ public function setPassword(string $password): void $this->password = $password; } + /** + * @return array|string|null + */ + public function getAuthentication(): array|string|null + { + if ($this->username !== null && $this->username !== '') { + return [$this->username, $this->password]; + } + return $this->password; + } + public function getSslContext(): ?SslContext { return $this->sslContext; diff --git a/src/RedisClusterOptionsFromIni.php b/src/RedisClusterOptionsFromIni.php index afdf574..f42dfaa 100644 --- a/src/RedisClusterOptionsFromIni.php +++ b/src/RedisClusterOptionsFromIni.php @@ -28,7 +28,7 @@ final class RedisClusterOptionsFromIni private array $readTimeoutByName; /** @var array */ - private array $authenticationByName; + private array $passwordByName; public function __construct() { @@ -88,16 +88,16 @@ public function __construct() $authenticationConfiguration = ''; } - $authenticationByName = []; + $passwordByName = []; if ($authenticationConfiguration !== '') { parse_str($authenticationConfiguration, $parsedAuthenticationByName); - foreach ($parsedAuthenticationByName as $name => $authentication) { - assert(is_string($name) && $name !== '' && is_string($authentication)); - $authenticationByName[$name] = $authentication; + foreach ($parsedAuthenticationByName as $name => $password) { + assert(is_string($name) && $name !== '' && is_string($password)); + $passwordByName[$name] = $password; } } - $this->authenticationByName = $authenticationByName; + $this->passwordByName = $passwordByName; } /** @@ -117,24 +117,24 @@ public function getSeeds(string $name): array /** * @psalm-param non-empty-string $name */ - public function getTimeout(string $name, float $fallback): float + public function getTimeout(string $name): ?float { - return $this->timeoutByName[$name] ?? $fallback; + return $this->timeoutByName[$name]; } /** * @psalm-param non-empty-string $name */ - public function getReadTimeout(string $name, float $fallback): float + public function getReadTimeout(string $name): ?float { - return $this->readTimeoutByName[$name] ?? $fallback; + return $this->readTimeoutByName[$name]; } /** * @psalm-param non-empty-string $name */ - public function getPasswordByName(string $name, string $fallback): string + public function getPassword(string $name): ?string { - return $this->authenticationByName[$name] ?? $fallback; + return $this->passwordByName[$name]; } } diff --git a/src/RedisClusterResourceManager.php b/src/RedisClusterResourceManager.php index 8fc5494..26290f3 100644 --- a/src/RedisClusterResourceManager.php +++ b/src/RedisClusterResourceManager.php @@ -54,21 +54,17 @@ private function createRedisResource(RedisClusterOptions $options): RedisCluster $options->getTimeout(), $options->getReadTimeout(), $options->isPersistent(), - $options->getPassword(), + $options->getAuthentication(), $options->getSslContext() ); } - $password = $options->getPassword(); - if ($password === '') { - $password = null; - } - /** - * Psalm currently (<= 5.23.1) uses an outdated (phpredis < 5.3.2) constructor signature for the RedisCluster + * Psalm currently (<= 5.26.1) uses an outdated (phpredis < 5.3.2) constructor signature for the RedisCluster * class in the phpredis extension. * - * @psalm-suppress TooManyArguments https://github.com/vimeo/psalm/pull/10862 + * @psalm-suppress TooManyArguments + * @psalm-suppress PossiblyInvalidArgument */ return new RedisClusterFromExtension( null, @@ -76,7 +72,7 @@ private function createRedisResource(RedisClusterOptions $options): RedisCluster $options->getTimeout(), $options->getReadTimeout(), $options->isPersistent(), - $password, + $options->getAuthentication(), $options->getSslContext()?->toSslContextArray() ); } @@ -89,7 +85,7 @@ private function createRedisResourceFromName( float $fallbackTimeout, float $fallbackReadTimeout, bool $persistent, - string $fallbackPassword, + array|string|null $fallbackAuthentication, ?SslContext $sslContext ): RedisClusterFromExtension { try { @@ -100,16 +96,17 @@ private function createRedisResourceFromName( throw new InvalidRedisClusterConfigurationException($throwable->getMessage(), previous: $throwable); } - $seeds = $options->getSeeds($name); - $timeout = $options->getTimeout($name, $fallbackTimeout); - $readTimeout = $options->getReadTimeout($name, $fallbackReadTimeout); - $password = $options->getPasswordByName($name, $fallbackPassword); + $seeds = $options->getSeeds($name); + $timeout = $options->getTimeout($name) ?? $fallbackTimeout; + $readTimeout = $options->getReadTimeout($name) ?? $fallbackReadTimeout; + $authentication = $options->getPassword($name) ?? $fallbackAuthentication; /** - * Psalm currently (<= 5.23.1) uses an outdated (phpredis < 5.3.2) constructor signature for the RedisCluster + * Psalm currently (<= 5.26.1) uses an outdated (phpredis < 5.3.2) constructor signature for the RedisCluster * class in the phpredis extension. * - * @psalm-suppress TooManyArguments https://github.com/vimeo/psalm/pull/10862 + * @psalm-suppress TooManyArguments + * @psalm-suppress PossiblyInvalidArgument */ return new RedisClusterFromExtension( null, @@ -117,7 +114,7 @@ private function createRedisResourceFromName( $timeout, $readTimeout, $persistent, - $password, + $authentication, $sslContext?->toSslContextArray() ); } diff --git a/test/unit/RedisClusterOptionsFromIniTest.php b/test/unit/RedisClusterOptionsFromIniTest.php index fbdc1a7..1f43575 100644 --- a/test/unit/RedisClusterOptionsFromIniTest.php +++ b/test/unit/RedisClusterOptionsFromIniTest.php @@ -74,9 +74,9 @@ public function testCanParseAllConfigurationsForName(): void $options = new RedisClusterOptionsFromIni(); self::assertEquals(['bar'], $options->getSeeds('foo')); - self::assertEquals(1.0, $options->getTimeout('foo', 0.0)); - self::assertEquals(2.0, $options->getReadTimeout('foo', 0.0)); - self::assertEquals('secret', $options->getPasswordByName('foo', '')); + self::assertEquals(1.0, $options->getTimeout('foo')); + self::assertEquals(2.0, $options->getReadTimeout('foo')); + self::assertEquals('secret', $options->getPassword('foo')); } protected function setUp(): void diff --git a/test/unit/RedisClusterOptionsTest.php b/test/unit/RedisClusterOptionsTest.php index e298bb6..77aa280 100644 --- a/test/unit/RedisClusterOptionsTest.php +++ b/test/unit/RedisClusterOptionsTest.php @@ -70,8 +70,9 @@ public function testCanHandleOptionsWithNodename(): void self::assertEquals('foo', $options->getName()); self::assertEquals(1.0, $options->getTimeout()); self::assertEquals(2.0, $options->getReadTimeout()); - self::assertEquals(false, $options->isPersistent()); + self::assertFalse($options->isPersistent()); self::assertEquals('1.0', $options->getRedisVersion()); + self::assertNull($options->getUsername()); self::assertEquals('secret', $options->getPassword()); } @@ -83,15 +84,16 @@ public function testCanHandleOptionsWithSeeds(): void 'read_timeout' => 2.0, 'persistent' => false, 'redis_version' => '1.0', - 'password' => 'secret', + 'username' => 'user', ]); self::assertEquals(['localhost:1234'], $options->getSeeds()); self::assertEquals(1.0, $options->getTimeout()); self::assertEquals(2.0, $options->getReadTimeout()); - self::assertEquals(false, $options->isPersistent()); + self::assertFalse($options->isPersistent()); self::assertEquals('1.0', $options->getRedisVersion()); - self::assertEquals('secret', $options->getPassword()); + self::assertEquals('user', $options->getUsername()); + self::assertNull($options->getPassword()); } public function testWillDetectSeedsAndNodenameConfiguration(): void From 11115cc83255ce818d230124304de8f936331e23 Mon Sep 17 00:00:00 2001 From: Robin Brabants Date: Wed, 30 Oct 2024 16:26:01 +0100 Subject: [PATCH 2/5] ensure backwards compatibility Signed-off-by: Robin Brabants --- src/RedisClusterOptions.php | 9 +++++---- src/RedisClusterOptionsFromIni.php | 12 ++++++------ src/RedisClusterResourceManager.php | 7 ++++--- test/unit/RedisClusterOptionsFromIniTest.php | 6 +++--- test/unit/RedisClusterOptionsTest.php | 2 +- 5 files changed, 19 insertions(+), 17 deletions(-) diff --git a/src/RedisClusterOptions.php b/src/RedisClusterOptions.php index 5f4c0a7..683c683 100644 --- a/src/RedisClusterOptions.php +++ b/src/RedisClusterOptions.php @@ -66,7 +66,7 @@ final class RedisClusterOptions extends AdapterOptions private ?string $username = null; - private ?string $password = null; + private string $password = ''; private ?SslContext $sslContext = null; @@ -263,7 +263,7 @@ public function setUsername(string $username): void $this->username = $username; } - public function getPassword(): ?string + public function getPassword(): string { return $this->password; } @@ -281,10 +281,11 @@ public function setPassword(string $password): void */ public function getAuthentication(): array|string|null { + $password = $this->password === '' ? null : $this->password; if ($this->username !== null && $this->username !== '') { - return [$this->username, $this->password]; + return [$this->username, $password]; } - return $this->password; + return $password; } public function getSslContext(): ?SslContext diff --git a/src/RedisClusterOptionsFromIni.php b/src/RedisClusterOptionsFromIni.php index f42dfaa..fbc4742 100644 --- a/src/RedisClusterOptionsFromIni.php +++ b/src/RedisClusterOptionsFromIni.php @@ -117,24 +117,24 @@ public function getSeeds(string $name): array /** * @psalm-param non-empty-string $name */ - public function getTimeout(string $name): ?float + public function getTimeout(string $name, float $fallback): float { - return $this->timeoutByName[$name]; + return $this->timeoutByName[$name] ?? $fallback; } /** * @psalm-param non-empty-string $name */ - public function getReadTimeout(string $name): ?float + public function getReadTimeout(string $name, float $fallback): float { - return $this->readTimeoutByName[$name]; + return $this->readTimeoutByName[$name] ?? $fallback; } /** * @psalm-param non-empty-string $name */ - public function getPassword(string $name): ?string + public function getPasswordByName(string $name, string $fallback): string { - return $this->passwordByName[$name]; + return $this->passwordByName[$name] ?? $fallback; } } diff --git a/src/RedisClusterResourceManager.php b/src/RedisClusterResourceManager.php index 26290f3..7b5114f 100644 --- a/src/RedisClusterResourceManager.php +++ b/src/RedisClusterResourceManager.php @@ -97,9 +97,10 @@ private function createRedisResourceFromName( } $seeds = $options->getSeeds($name); - $timeout = $options->getTimeout($name) ?? $fallbackTimeout; - $readTimeout = $options->getReadTimeout($name) ?? $fallbackReadTimeout; - $authentication = $options->getPassword($name) ?? $fallbackAuthentication; + $timeout = $options->getTimeout($name, $fallbackTimeout); + $readTimeout = $options->getReadTimeout($name, $fallbackReadTimeout); + $password = $options->getPasswordByName($name, ''); + $authentication = $password === '' ? $fallbackAuthentication : $password; /** * Psalm currently (<= 5.26.1) uses an outdated (phpredis < 5.3.2) constructor signature for the RedisCluster diff --git a/test/unit/RedisClusterOptionsFromIniTest.php b/test/unit/RedisClusterOptionsFromIniTest.php index 1f43575..fbdc1a7 100644 --- a/test/unit/RedisClusterOptionsFromIniTest.php +++ b/test/unit/RedisClusterOptionsFromIniTest.php @@ -74,9 +74,9 @@ public function testCanParseAllConfigurationsForName(): void $options = new RedisClusterOptionsFromIni(); self::assertEquals(['bar'], $options->getSeeds('foo')); - self::assertEquals(1.0, $options->getTimeout('foo')); - self::assertEquals(2.0, $options->getReadTimeout('foo')); - self::assertEquals('secret', $options->getPassword('foo')); + self::assertEquals(1.0, $options->getTimeout('foo', 0.0)); + self::assertEquals(2.0, $options->getReadTimeout('foo', 0.0)); + self::assertEquals('secret', $options->getPasswordByName('foo', '')); } protected function setUp(): void diff --git a/test/unit/RedisClusterOptionsTest.php b/test/unit/RedisClusterOptionsTest.php index 77aa280..c884956 100644 --- a/test/unit/RedisClusterOptionsTest.php +++ b/test/unit/RedisClusterOptionsTest.php @@ -93,7 +93,7 @@ public function testCanHandleOptionsWithSeeds(): void self::assertFalse($options->isPersistent()); self::assertEquals('1.0', $options->getRedisVersion()); self::assertEquals('user', $options->getUsername()); - self::assertNull($options->getPassword()); + self::assertEquals('', $options->getPassword()); } public function testWillDetectSeedsAndNodenameConfiguration(): void From 92de643e856d3586d35ff67d3b962c3a784cea54 Mon Sep 17 00:00:00 2001 From: Robin Brabants Date: Wed, 20 Nov 2024 21:37:57 +0100 Subject: [PATCH 3/5] refactor getting authentication details for the Redis cluster Signed-off-by: Robin Brabants --- psalm-baseline.xml | 2 +- ...alidRedisClusterConfigurationException.php | 5 ++++ src/RedisClusterOptions.php | 24 +++++-------------- src/RedisClusterResourceManager.php | 22 +++++++++++++---- test/unit/RedisClusterOptionsTest.php | 6 ++--- 5 files changed, 33 insertions(+), 26 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index c579c18..72cfeed 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -52,7 +52,7 @@ - + diff --git a/src/Exception/InvalidRedisClusterConfigurationException.php b/src/Exception/InvalidRedisClusterConfigurationException.php index 735093c..3909523 100644 --- a/src/Exception/InvalidRedisClusterConfigurationException.php +++ b/src/Exception/InvalidRedisClusterConfigurationException.php @@ -42,4 +42,9 @@ public static function fromInvalidSeedsConfiguration(string $seed): self ) ); } + + public static function fromMissingRequiredPassword(): self + { + return new self('If a Redis user is provided, a password has to be configured as well.'); + } } diff --git a/src/RedisClusterOptions.php b/src/RedisClusterOptions.php index 683c683..fb92752 100644 --- a/src/RedisClusterOptions.php +++ b/src/RedisClusterOptions.php @@ -64,7 +64,7 @@ final class RedisClusterOptions extends AdapterOptions /** @psalm-var array */ private array $libOptions = []; - private ?string $username = null; + private string $user = ''; private string $password = ''; @@ -250,17 +250,17 @@ public function getLibOption(int $option, mixed $default = null): mixed return $this->libOptions[$option] ?? $default; } - public function getUsername(): ?string + public function getUser(): string { - return $this->username; + return $this->user; } /** - * @psalm-param non-empty-string $username + * @psalm-param non-empty-string $user */ - public function setUsername(string $username): void + public function setUser(string $user): void { - $this->username = $username; + $this->user = $user; } public function getPassword(): string @@ -276,18 +276,6 @@ public function setPassword(string $password): void $this->password = $password; } - /** - * @return array|string|null - */ - public function getAuthentication(): array|string|null - { - $password = $this->password === '' ? null : $this->password; - if ($this->username !== null && $this->username !== '') { - return [$this->username, $password]; - } - return $password; - } - public function getSslContext(): ?SslContext { return $this->sslContext; diff --git a/src/RedisClusterResourceManager.php b/src/RedisClusterResourceManager.php index 7b5114f..69e1fe4 100644 --- a/src/RedisClusterResourceManager.php +++ b/src/RedisClusterResourceManager.php @@ -48,13 +48,26 @@ public function getResource(): RedisClusterFromExtension private function createRedisResource(RedisClusterOptions $options): RedisClusterFromExtension { + $user = $options->getUser(); + $password = $options->getPassword(); + + if ($user !== '' && $password !== '') { + $authentication = [$user, $password]; + } elseif ($user !== '' && $password === '') { + throw InvalidRedisClusterConfigurationException::fromMissingRequiredPassword(); + } elseif ($user === '' && $password !== '') { + $authentication = [$password]; + } else { + $authentication = null; + } + if ($options->hasName()) { return $this->createRedisResourceFromName( $options->getName(), $options->getTimeout(), $options->getReadTimeout(), $options->isPersistent(), - $options->getAuthentication(), + $authentication, $options->getSslContext() ); } @@ -64,7 +77,7 @@ private function createRedisResource(RedisClusterOptions $options): RedisCluster * class in the phpredis extension. * * @psalm-suppress TooManyArguments - * @psalm-suppress PossiblyInvalidArgument + * @psalm-suppress InvalidArgument */ return new RedisClusterFromExtension( null, @@ -72,20 +85,21 @@ private function createRedisResource(RedisClusterOptions $options): RedisCluster $options->getTimeout(), $options->getReadTimeout(), $options->isPersistent(), - $options->getAuthentication(), + $authentication, $options->getSslContext()?->toSslContextArray() ); } /** * @psalm-param non-empty-string $name + * @psalm-param array{0: non-empty-string, 1?: non-empty-string}|null $fallbackAuthentication */ private function createRedisResourceFromName( string $name, float $fallbackTimeout, float $fallbackReadTimeout, bool $persistent, - array|string|null $fallbackAuthentication, + array|null $fallbackAuthentication, ?SslContext $sslContext ): RedisClusterFromExtension { try { diff --git a/test/unit/RedisClusterOptionsTest.php b/test/unit/RedisClusterOptionsTest.php index c884956..71e80eb 100644 --- a/test/unit/RedisClusterOptionsTest.php +++ b/test/unit/RedisClusterOptionsTest.php @@ -72,7 +72,7 @@ public function testCanHandleOptionsWithNodename(): void self::assertEquals(2.0, $options->getReadTimeout()); self::assertFalse($options->isPersistent()); self::assertEquals('1.0', $options->getRedisVersion()); - self::assertNull($options->getUsername()); + self::assertEmpty($options->getUser()); self::assertEquals('secret', $options->getPassword()); } @@ -84,7 +84,7 @@ public function testCanHandleOptionsWithSeeds(): void 'read_timeout' => 2.0, 'persistent' => false, 'redis_version' => '1.0', - 'username' => 'user', + 'user' => 'user', ]); self::assertEquals(['localhost:1234'], $options->getSeeds()); @@ -92,7 +92,7 @@ public function testCanHandleOptionsWithSeeds(): void self::assertEquals(2.0, $options->getReadTimeout()); self::assertFalse($options->isPersistent()); self::assertEquals('1.0', $options->getRedisVersion()); - self::assertEquals('user', $options->getUsername()); + self::assertEquals('user', $options->getUser()); self::assertEquals('', $options->getPassword()); } From 6b2cf3d8a121e30bb20befb36d5a48a78d7c2b6c Mon Sep 17 00:00:00 2001 From: Robin Brabants Date: Thu, 21 Nov 2024 18:13:03 +0100 Subject: [PATCH 4/5] extract Redis authentication object creation to a utility class and use for the RedisResourceManager as well Signed-off-by: Robin Brabants --- src/RedisAuthProvider.php | 35 +++++++++++ src/RedisClusterResourceManager.php | 13 +---- src/RedisResourceManager.php | 16 +---- test/unit/RedisAuthProviderTest.php | 91 +++++++++++++++++++++++++++++ 4 files changed, 128 insertions(+), 27 deletions(-) create mode 100644 src/RedisAuthProvider.php create mode 100644 test/unit/RedisAuthProviderTest.php diff --git a/src/RedisAuthProvider.php b/src/RedisAuthProvider.php new file mode 100644 index 0000000..200bcce --- /dev/null +++ b/src/RedisAuthProvider.php @@ -0,0 +1,35 @@ +getUser(); - $password = $options->getPassword(); - - if ($user !== '' && $password !== '') { - $authentication = [$user, $password]; - } elseif ($user !== '' && $password === '') { - throw InvalidRedisClusterConfigurationException::fromMissingRequiredPassword(); - } elseif ($user === '' && $password !== '') { - $authentication = [$password]; - } else { - $authentication = null; - } + $authentication = RedisAuthProvider::createAuthenticationObject($options->getUser(), $options->getPassword()); if ($options->hasName()) { return $this->createRedisResourceFromName( diff --git a/src/RedisResourceManager.php b/src/RedisResourceManager.php index 0000780..b1f8615 100644 --- a/src/RedisResourceManager.php +++ b/src/RedisResourceManager.php @@ -57,21 +57,7 @@ private function createRedisFromExtension(RedisOptions $options): RedisFromExten $port = $server['port'] ?? self::DEFAULT_REDIS_PORT; } - $authentication = []; - $user = $options->getUser(); - $password = $options->getPassword(); - - if ($user !== null) { - $authentication[] = $user; - } - - if ($password !== null) { - $authentication[] = $password; - } - - if ($authentication === []) { - $authentication = null; - } + $authentication = RedisAuthProvider::createAuthenticationObject($options->getUser(), $options->getPassword()); $resourceOptions = [ 'host' => $host, diff --git a/test/unit/RedisAuthProviderTest.php b/test/unit/RedisAuthProviderTest.php new file mode 100644 index 0000000..edcd504 --- /dev/null +++ b/test/unit/RedisAuthProviderTest.php @@ -0,0 +1,91 @@ +expectException(InvalidRedisClusterConfigurationException::class); + $this->expectExceptionMessage( + InvalidRedisClusterConfigurationException::fromMissingRequiredPassword()->getMessage() + ); + RedisAuthProvider::createAuthenticationObject($user, $password); + } + + /** + * @dataProvider validAuthenticationProvider + */ + public function testValidUserAndPasswordProvided( + ?string $user, + ?string $password, + array|null $expectedAuthentication + ): void { + $actualAuthentication = RedisAuthProvider::createAuthenticationObject($user, $password); + $this->assertEquals($expectedAuthentication, $actualAuthentication); + } + + /** + * @psalm-return non-empty-array + */ + public static function validAuthenticationProvider(): array + { + return [ + 'user and password' => [ + self::DUMMY_USER, + self::DUMMY_PASSWORD, + [self::DUMMY_USER, self::DUMMY_PASSWORD], + ], + 'only password (user is empty string)' => [ + '', + self::DUMMY_PASSWORD, + [self::DUMMY_PASSWORD], + ], + 'no authentication provided (empty strings)' => [ + '', + '', + null, + ], + 'only password (user is null)' => [ + null, + self::DUMMY_PASSWORD, + [self::DUMMY_PASSWORD], + ], + 'no authentication provided (null values)' => [ + null, + null, + null, + ], + ]; + } + + /** + * @psalm-return non-empty-array + */ + public static function invalidAuthenticationProvider(): array + { + return [ + 'user without a password (password is empty string)' => [ + self::DUMMY_USER, + '', + ], + 'user without a password (password is null)' => [ + self::DUMMY_USER, + null, + ], + ]; + } +} From 71b69e1be3d9efb3b1b7f89f9b2f756d2ca068b1 Mon Sep 17 00:00:00 2001 From: Robin Brabants Date: Mon, 25 Nov 2024 13:16:43 +0100 Subject: [PATCH 5/5] refactoring of the authentication helper Signed-off-by: Robin Brabants --- .gitignore | 3 +- psalm-baseline.xml | 5 - ...alidRedisClusterConfigurationException.php | 5 - src/RedisAuthProvider.php | 35 ------ src/RedisAuthenticationInfo.php | 51 +++++++++ src/RedisClusterOptionsFromIni.php | 16 ++- src/RedisClusterResourceManager.php | 11 +- src/RedisOptions.php | 6 ++ src/RedisResourceManager.php | 4 +- test/unit/RedisAuthProviderTest.php | 91 ---------------- test/unit/RedisAuthenticationInfoTest.php | 100 ++++++++++++++++++ 11 files changed, 177 insertions(+), 150 deletions(-) delete mode 100644 src/RedisAuthProvider.php create mode 100644 src/RedisAuthenticationInfo.php delete mode 100644 test/unit/RedisAuthProviderTest.php create mode 100644 test/unit/RedisAuthenticationInfoTest.php diff --git a/.gitignore b/.gitignore index 31e7bde..4900ea6 100644 --- a/.gitignore +++ b/.gitignore @@ -1,7 +1,8 @@ -/.phpunit.result.cache +/.phpunit.cache /.psalm-cache /docs/html/ /laminas-mkdoc-theme.tgz /laminas-mkdoc-theme/ /vendor/ /.phpcs-cache +/.idea diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 72cfeed..a92323f 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1,10 +1,5 @@ - - - - - diff --git a/src/Exception/InvalidRedisClusterConfigurationException.php b/src/Exception/InvalidRedisClusterConfigurationException.php index 3909523..735093c 100644 --- a/src/Exception/InvalidRedisClusterConfigurationException.php +++ b/src/Exception/InvalidRedisClusterConfigurationException.php @@ -42,9 +42,4 @@ public static function fromInvalidSeedsConfiguration(string $seed): self ) ); } - - public static function fromMissingRequiredPassword(): self - { - return new self('If a Redis user is provided, a password has to be configured as well.'); - } } diff --git a/src/RedisAuthProvider.php b/src/RedisAuthProvider.php deleted file mode 100644 index 200bcce..0000000 --- a/src/RedisAuthProvider.php +++ /dev/null @@ -1,35 +0,0 @@ -getUser(); + $password = $options->getPassword(); + + if ($password === null || $password === '') { + return null; + } + + if ($username === null || $username === '') { + return new self(null, $password); + } + + return new self($username, $password); + } + + /** + * @see https://github.com/phpredis/phpredis/blob/4cd3f59356582a65aec1cceed44741bd5d161d9e/library.c#L4382 + * + * @return array{0:non-empty-string,1?:non-empty-string} + */ + public function toRedisAuthInfo(): array + { + if ($this->username === null) { + return [$this->password]; + } + + return [$this->username, $this->password]; + } +} diff --git a/src/RedisClusterOptionsFromIni.php b/src/RedisClusterOptionsFromIni.php index fbc4742..da62529 100644 --- a/src/RedisClusterOptionsFromIni.php +++ b/src/RedisClusterOptionsFromIni.php @@ -4,6 +4,7 @@ namespace Laminas\Cache\Storage\Adapter; +use InvalidArgumentException; use Laminas\Cache\Storage\Adapter\Exception\InvalidRedisClusterConfigurationException; use Webmozart\Assert\Assert; @@ -43,12 +44,17 @@ public function __construct() $seedsByName = []; parse_str($seedsConfiguration, $parsedSeedsByName); - foreach ($parsedSeedsByName as $name => $seeds) { - assert(is_string($name) && $name !== ''); - Assert::isNonEmptyList($seeds); - Assert::allStringNotEmpty($seeds); - $seedsByName[$name] = $seeds; + try { + foreach ($parsedSeedsByName as $name => $seeds) { + assert(is_string($name) && $name !== ''); + Assert::isNonEmptyList($seeds); + Assert::allStringNotEmpty($seeds); + $seedsByName[$name] = $seeds; + } + } catch (InvalidArgumentException) { + throw InvalidRedisClusterConfigurationException::fromInvalidSeedsConfiguration($seedsConfiguration); } + $this->seedsByName = $seedsByName; $timeoutConfiguration = ini_get('redis.clusters.timeout'); diff --git a/src/RedisClusterResourceManager.php b/src/RedisClusterResourceManager.php index ddd9a41..bfcd364 100644 --- a/src/RedisClusterResourceManager.php +++ b/src/RedisClusterResourceManager.php @@ -48,7 +48,7 @@ public function getResource(): RedisClusterFromExtension private function createRedisResource(RedisClusterOptions $options): RedisClusterFromExtension { - $authentication = RedisAuthProvider::createAuthenticationObject($options->getUser(), $options->getPassword()); + $authenticationInfo = RedisAuthenticationInfo::fromOptions($options); if ($options->hasName()) { return $this->createRedisResourceFromName( @@ -56,7 +56,7 @@ private function createRedisResource(RedisClusterOptions $options): RedisCluster $options->getTimeout(), $options->getReadTimeout(), $options->isPersistent(), - $authentication, + $authenticationInfo, $options->getSslContext() ); } @@ -74,21 +74,20 @@ private function createRedisResource(RedisClusterOptions $options): RedisCluster $options->getTimeout(), $options->getReadTimeout(), $options->isPersistent(), - $authentication, + $authenticationInfo?->toRedisAuthInfo(), $options->getSslContext()?->toSslContextArray() ); } /** * @psalm-param non-empty-string $name - * @psalm-param array{0: non-empty-string, 1?: non-empty-string}|null $fallbackAuthentication */ private function createRedisResourceFromName( string $name, float $fallbackTimeout, float $fallbackReadTimeout, bool $persistent, - array|null $fallbackAuthentication, + RedisAuthenticationInfo|null $fallbackAuthentication, ?SslContext $sslContext ): RedisClusterFromExtension { try { @@ -103,7 +102,7 @@ private function createRedisResourceFromName( $timeout = $options->getTimeout($name, $fallbackTimeout); $readTimeout = $options->getReadTimeout($name, $fallbackReadTimeout); $password = $options->getPasswordByName($name, ''); - $authentication = $password === '' ? $fallbackAuthentication : $password; + $authentication = $password === '' ? $fallbackAuthentication?->toRedisAuthInfo() : $password; /** * Psalm currently (<= 5.26.1) uses an outdated (phpredis < 5.3.2) constructor signature for the RedisCluster diff --git a/src/RedisOptions.php b/src/RedisOptions.php index db99951..704a5b3 100644 --- a/src/RedisOptions.php +++ b/src/RedisOptions.php @@ -228,6 +228,9 @@ public function setPassword(string $password): void $this->password = $password; } + /** + * @return non-empty-string|null + */ public function getPassword(): string|null { return $this->password; @@ -242,6 +245,9 @@ public function setUser(string $user): void $this->user = $user; } + /** + * @return non-empty-string|null + */ public function getUser(): ?string { return $this->user; diff --git a/src/RedisResourceManager.php b/src/RedisResourceManager.php index b1f8615..d2ddd38 100644 --- a/src/RedisResourceManager.php +++ b/src/RedisResourceManager.php @@ -57,14 +57,14 @@ private function createRedisFromExtension(RedisOptions $options): RedisFromExten $port = $server['port'] ?? self::DEFAULT_REDIS_PORT; } - $authentication = RedisAuthProvider::createAuthenticationObject($options->getUser(), $options->getPassword()); + $authenticationInfo = RedisAuthenticationInfo::fromOptions($options); $resourceOptions = [ 'host' => $host, 'port' => $port, 'connectTimeout' => $server['timeout'] ?? null, 'persistent' => $options->getPersistentId() ?? $options->isPersistent(), - 'auth' => $authentication, + 'auth' => $authenticationInfo?->toRedisAuthInfo(), ]; $resource = new RedisFromExtension(array_filter($resourceOptions, fn (mixed $value) => $value !== null)); diff --git a/test/unit/RedisAuthProviderTest.php b/test/unit/RedisAuthProviderTest.php deleted file mode 100644 index edcd504..0000000 --- a/test/unit/RedisAuthProviderTest.php +++ /dev/null @@ -1,91 +0,0 @@ -expectException(InvalidRedisClusterConfigurationException::class); - $this->expectExceptionMessage( - InvalidRedisClusterConfigurationException::fromMissingRequiredPassword()->getMessage() - ); - RedisAuthProvider::createAuthenticationObject($user, $password); - } - - /** - * @dataProvider validAuthenticationProvider - */ - public function testValidUserAndPasswordProvided( - ?string $user, - ?string $password, - array|null $expectedAuthentication - ): void { - $actualAuthentication = RedisAuthProvider::createAuthenticationObject($user, $password); - $this->assertEquals($expectedAuthentication, $actualAuthentication); - } - - /** - * @psalm-return non-empty-array - */ - public static function validAuthenticationProvider(): array - { - return [ - 'user and password' => [ - self::DUMMY_USER, - self::DUMMY_PASSWORD, - [self::DUMMY_USER, self::DUMMY_PASSWORD], - ], - 'only password (user is empty string)' => [ - '', - self::DUMMY_PASSWORD, - [self::DUMMY_PASSWORD], - ], - 'no authentication provided (empty strings)' => [ - '', - '', - null, - ], - 'only password (user is null)' => [ - null, - self::DUMMY_PASSWORD, - [self::DUMMY_PASSWORD], - ], - 'no authentication provided (null values)' => [ - null, - null, - null, - ], - ]; - } - - /** - * @psalm-return non-empty-array - */ - public static function invalidAuthenticationProvider(): array - { - return [ - 'user without a password (password is empty string)' => [ - self::DUMMY_USER, - '', - ], - 'user without a password (password is null)' => [ - self::DUMMY_USER, - null, - ], - ]; - } -} diff --git a/test/unit/RedisAuthenticationInfoTest.php b/test/unit/RedisAuthenticationInfoTest.php new file mode 100644 index 0000000..1bdb4ee --- /dev/null +++ b/test/unit/RedisAuthenticationInfoTest.php @@ -0,0 +1,100 @@ + $user, 'password' => $password], + fn($element) => $element !== null + )); + /** @psalm-suppress InternalMethod,InternalClass We are explicitly testing an internal method here */ + $actualAuthentication = RedisAuthenticationInfo::fromOptions($options); + /** @psalm-suppress InternalMethod We are explicitly testing an internal method here */ + self::assertEquals($expectedAuthentication, $actualAuthentication?->toRedisAuthInfo()); + } + + #[DataProvider('authenticationInfo')] + public function testUserAndPasswordCombinationsForRedisCluster( + ?string $user, + ?string $password, + array|null $expectedAuthentication + ): void { + $options = new RedisClusterOptions(array_filter( + ['user' => $user, 'password' => $password, 'name' => 'test'], + fn($element) => $element !== null + )); + /** @psalm-suppress InternalMethod,InternalClass We are explicitly testing an internal method here */ + $actualAuthentication = RedisAuthenticationInfo::fromOptions($options); + /** @psalm-suppress InternalMethod We are explicitly testing an internal method here */ + self::assertEquals($expectedAuthentication, $actualAuthentication?->toRedisAuthInfo()); + } + + /** + * @psalm-suppress PossiblyUnusedMethod PHPUnit psalm plugin does not yet support attributes + * @psalm-return non-empty-array + */ + public static function authenticationInfo(): array + { + return [ + 'user and password' => [ + self::DUMMY_USER, + self::DUMMY_PASSWORD, + [self::DUMMY_USER, self::DUMMY_PASSWORD], + ], + 'only password (user is empty string)' => [ + '', + self::DUMMY_PASSWORD, + [self::DUMMY_PASSWORD], + ], + 'only password (user is null)' => [ + null, + self::DUMMY_PASSWORD, + [self::DUMMY_PASSWORD], + ], + 'no authentication provided (empty strings)' => [ + '', + '', + null, + ], + 'no authentication provided (null values)' => [ + null, + null, + null, + ], + 'user without a password (password is empty string)' => [ + self::DUMMY_USER, + '', + null, + ], + 'user without a password (password is null)' => [ + self::DUMMY_USER, + null, + null, + ], + ]; + } +}