Add support for specific username setting in RedisCluster

[robin-brabants]

Q	A
Documentation	no
Bugfix	no
BC Break	no
New Feature	yes
RFC	no
QA	no

Description

This small feature allows to pass a username to the RedisCluster constructor from the phpredis extension in order to be able to authenticate with a non-default user. Even though the README does not mention it, passing username and password in the form of an array is supported, see:

• https://github.com/phpredis/phpredis/blob/develop/redis_cluster.stub.php#L50
• https://github.com/phpredis/phpredis/blob/develop/library.c#L4409

Without this feature, one could not connect to a cluster using a user other than the one named 'default'. When using RBAC it is recommended not to connect with the default user anymore by Redis.

This change should be backwards compatible in the sense that if no username option is specified, the phpredis extension will still connect with the default user.

Note: the stubs in Psalm are outdated: https://github.com/phpredis/phpredis/blob/develop/redis_cluster.stub.php#L50

[robin-brabants]

@boesing
When you would find the time, could you maybe have a short look at this PR?

[boesing]

FYI: I am not doing laminas work for the unforseeable future. I switched jobs and do not work with laminas anymore.

I might help you with this PR but please do not expect me to respond in a meaningful timeframe.

[boesing]

Oh, and please try to synchronize some stuff with #92 which introduced the user option for Redis.

At least from the naming perspective, that should be synchronized and this can be used for the authentication variable refactoring.

[robin-brabants]

Thanks a lot @boesing for your review even though you are not working with Laminas anymore, I really appreciate this and of course understand that you cannot respond that quickly anymore then

[robin-brabants]

Oh, and please try to synchronize some stuff with #92 which introduced the user option for Redis.

At least from the naming perspective, that should be synchronized and this can be used for the authentication variable refactoring.

I am synchronizing with the options for simple Redis nodes, but I can e.g. not change the password to be a nullable field otherwise I would introduce a breaking change in the getter. I guess it would be best then to at least align things within the RedisClusterOptions and default the user field to an empty string and make it not nullable.

I will rename 'username' to 'user' though to align with the Redis options.

I cannot 100% take over the solution for authentication variable from

laminas-cache-storage-adapter-redis/src/RedisResourceManager.php

Lines 60 to 74 in cf93d7f

	$authentication = [];
	$user = $options->getUser();
	$password = $options->getPassword();
	if ($user !== null) {
	$authentication[] = $user;
	}
	if ($password !== null) {
	$authentication[] = $password;
	}
	if ($authentication === []) {
	$authentication = null;
	}

though. The issue is that if you only specify a user and no password, you get an array with only the username (e.g. ['my_user']) which would then actually be interpreted as the password instead of the user by phpRedis: https://github.com/phpredis/phpredis/blob/develop/library.c#L4421-L4422

[boesing]

I've refactored the authentication helper a bit but unfortunately, I am not allowed to push to your forks branch.
Feel free to apply this patch to your branch and let me know what you think.

Subject: [PATCH] chore: add `.phpunit.cache` directory to `.gitignore`
refactor: convert `RedisAuthProvider` to actual `RedisAuthenticationInfo`
---
Index: src/Exception/InvalidRedisClusterConfigurationException.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/Exception/InvalidRedisClusterConfigurationException.php b/src/Exception/InvalidRedisClusterConfigurationException.php
--- a/src/Exception/InvalidRedisClusterConfigurationException.php	(revision 6b2cf3d8a121e30bb20befb36d5a48a78d7c2b6c)
+++ b/src/Exception/InvalidRedisClusterConfigurationException.php	(revision c4ad9bf2397159c4a347b2a9387110c4e273d5b0)
@@ -42,9 +42,4 @@
             )
         );
     }
-
-    public static function fromMissingRequiredPassword(): self
-    {
-        return new self('If a Redis user is provided, a password has to be configured as well.');
-    }
 }
Index: src/RedisAuthenticationInfo.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/RedisAuthenticationInfo.php b/src/RedisAuthenticationInfo.php
new file mode 100644
--- /dev/null	(revision c4ad9bf2397159c4a347b2a9387110c4e273d5b0)
+++ b/src/RedisAuthenticationInfo.php	(revision c4ad9bf2397159c4a347b2a9387110c4e273d5b0)
@@ -0,0 +1,67 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Laminas\Cache\Storage\Adapter;
+
+/**
+ * @internal
+ */
+final class RedisAuthenticationInfo
+{
+    /**
+     * @param non-empty-string|null $username
+     * @param non-empty-string $password
+     */
+    private function __construct(
+        private readonly string|null $username,
+        private readonly string $password,
+    ) {
+    }
+
+    public static function fromRedisClusterOptions(RedisClusterOptions $options): self|null
+    {
+        $username = $options->getUser();
+        $password = $options->getPassword();
+
+        if ($password === '') {
+            return null;
+        }
+
+        if ($username === '') {
+            return new self(null, $password);
+        }
+
+        return new self($username, $password);
+    }
+
+    public static function fromRedisOptions(RedisOptions $options): self|null
+    {
+        $username = $options->getUser();
+        $password = $options->getPassword();
+
+        if ($password === null) {
+            return null;
+        }
+
+        if ($username === null) {
+            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];
+    }
+}
Index: src/RedisClusterOptionsFromIni.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/RedisClusterOptionsFromIni.php b/src/RedisClusterOptionsFromIni.php
--- a/src/RedisClusterOptionsFromIni.php	(revision 6b2cf3d8a121e30bb20befb36d5a48a78d7c2b6c)
+++ b/src/RedisClusterOptionsFromIni.php	(revision c4ad9bf2397159c4a347b2a9387110c4e273d5b0)
@@ -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 @@
 
         $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');
Index: src/RedisClusterResourceManager.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/RedisClusterResourceManager.php b/src/RedisClusterResourceManager.php
--- a/src/RedisClusterResourceManager.php	(revision 6b2cf3d8a121e30bb20befb36d5a48a78d7c2b6c)
+++ b/src/RedisClusterResourceManager.php	(revision c4ad9bf2397159c4a347b2a9387110c4e273d5b0)
@@ -48,7 +48,7 @@
 
     private function createRedisResource(RedisClusterOptions $options): RedisClusterFromExtension
     {
-        $authentication = RedisAuthProvider::createAuthenticationObject($options->getUser(), $options->getPassword());
+        $authentication = RedisAuthenticationInfo::fromRedisClusterOptions($options);
 
         if ($options->hasName()) {
             return $this->createRedisResourceFromName(
@@ -74,21 +74,20 @@
             $options->getTimeout(),
             $options->getReadTimeout(),
             $options->isPersistent(),
-            $authentication,
+            $authentication?->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 @@
         $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
Index: src/RedisOptions.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/RedisOptions.php b/src/RedisOptions.php
--- a/src/RedisOptions.php	(revision 6b2cf3d8a121e30bb20befb36d5a48a78d7c2b6c)
+++ b/src/RedisOptions.php	(revision c4ad9bf2397159c4a347b2a9387110c4e273d5b0)
@@ -228,6 +228,9 @@
         $this->password = $password;
     }
 
+    /**
+     * @return non-empty-string|null
+     */
     public function getPassword(): string|null
     {
         return $this->password;
@@ -242,6 +245,9 @@
         $this->user = $user;
     }
 
+    /**
+     * @return non-empty-string|null
+     */
     public function getUser(): ?string
     {
         return $this->user;
Index: src/RedisResourceManager.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/RedisResourceManager.php b/src/RedisResourceManager.php
--- a/src/RedisResourceManager.php	(revision 6b2cf3d8a121e30bb20befb36d5a48a78d7c2b6c)
+++ b/src/RedisResourceManager.php	(revision c4ad9bf2397159c4a347b2a9387110c4e273d5b0)
@@ -57,14 +57,14 @@
             $port = $server['port'] ?? self::DEFAULT_REDIS_PORT;
         }
 
-        $authentication = RedisAuthProvider::createAuthenticationObject($options->getUser(), $options->getPassword());
+        $authentication = RedisAuthenticationInfo::fromRedisOptions($options);
 
         $resourceOptions = [
             'host'           => $host,
             'port'           => $port,
             'connectTimeout' => $server['timeout'] ?? null,
             'persistent'     => $options->getPersistentId() ?? $options->isPersistent(),
-            'auth'           => $authentication,
+            'auth'           => $authentication?->toRedisAuthInfo(),
         ];
 
         $resource = new RedisFromExtension(array_filter($resourceOptions, fn (mixed $value) => $value !== null));
Index: test/unit/RedisAuthProviderTest.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/unit/RedisAuthProviderTest.php b/test/unit/RedisAuthProviderTest.php
--- a/test/unit/RedisAuthProviderTest.php	(revision 6b2cf3d8a121e30bb20befb36d5a48a78d7c2b6c)
+++ b/test/unit/RedisAuthProviderTest.php	(revision c4ad9bf2397159c4a347b2a9387110c4e273d5b0)
@@ -4,44 +4,40 @@
 
 namespace LaminasTest\Cache\Storage\Adapter;
 
-use Laminas\Cache\Storage\Adapter\Exception\InvalidRedisClusterConfigurationException;
-use Laminas\Cache\Storage\Adapter\RedisAuthProvider;
+use Laminas\Cache\Storage\Adapter\RedisAuthenticationInfo;
+use Laminas\Cache\Storage\Adapter\RedisOptions;
+use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\TestCase;
 
+use function array_filter;
+
 final class RedisAuthProviderTest extends TestCase
 {
     private const DUMMY_USER     = 'user';
     private const DUMMY_PASSWORD = 'password';
 
-    /**
-     * @dataProvider invalidAuthenticationProvider
-     */
-    public function testWillThrowExceptionWhenUserWithoutPasswordProvided(string $user, ?string $password): void
-    {
-        $this->expectException(InvalidRedisClusterConfigurationException::class);
-        $this->expectExceptionMessage(
-            InvalidRedisClusterConfigurationException::fromMissingRequiredPassword()->getMessage()
-        );
-        RedisAuthProvider::createAuthenticationObject($user, $password);
-    }
-
-    /**
-     * @dataProvider validAuthenticationProvider
-     */
-    public function testValidUserAndPasswordProvided(
+    #[DataProvider('validAuthenticationInfo')]
+    public function testValidRedisUserAndPasswordCombinations(
         ?string $user,
         ?string $password,
         array|null $expectedAuthentication
     ): void {
-        $actualAuthentication = RedisAuthProvider::createAuthenticationObject($user, $password);
-        $this->assertEquals($expectedAuthentication, $actualAuthentication);
+        $options = new RedisOptions(array_filter(['user' => $user, 'password' => $password]));
+        /** @psalm-suppress InternalMethod,InternalClass We are explicitly testing internal method here */
+        $actualAuthentication = RedisAuthenticationInfo::fromRedisOptions($options);
+        /** @psalm-suppress InternalMethod We are explicitly testing internal method here */
+        self::assertEquals($expectedAuthentication, $actualAuthentication?->toRedisAuthInfo());
     }
 
     /**
-     * @psalm-return non-empty-array<non-empty-string,array{0:string|null,1:string|null,
-     * 2:array{0: non-empty-string, 1?: non-empty-string}|null}>
+     * @psalm-suppress PossiblyUnusedMethod PHPUnit psalm plugin does not yet support attributes
+     * @psalm-return non-empty-array<non-empty-string, array{
+     *  0:string|null,
+     *  1:string|null,
+     *  2: array{0: non-empty-string, 1?: non-empty-string}|null
+     * }>
      */
-    public static function validAuthenticationProvider(): array
+    public static function validAuthenticationInfo(): array
     {
         return [
             'user and password'                          => [
@@ -71,21 +67,4 @@
             ],
         ];
     }
-
-    /**
-     * @psalm-return non-empty-array<non-empty-string,array{0:non-empty-string,1:string|null}>
-     */
-    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,
-            ],
-        ];
-    }
 }
Index: .gitignore
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/.gitignore b/.gitignore
--- a/.gitignore	(revision c4ad9bf2397159c4a347b2a9387110c4e273d5b0)
+++ b/.gitignore	(revision f7dc15a8cc079aefbedcd0fadac46d8ed0c8d4ba)
@@ -1,4 +1,4 @@
-/.phpunit.result.cache
+/.phpunit.cache
 /.psalm-cache
 /docs/html/
 /laminas-mkdoc-theme.tgz
Index: src/RedisAuthProvider.php
===================================================================
diff --git a/src/RedisAuthProvider.php b/src/RedisAuthProvider.php
deleted file mode 100644
--- a/src/RedisAuthProvider.php	(revision 6b2cf3d8a121e30bb20befb36d5a48a78d7c2b6c)
+++ /dev/null	(revision 6b2cf3d8a121e30bb20befb36d5a48a78d7c2b6c)
@@ -1,35 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace Laminas\Cache\Storage\Adapter;
-
-use Laminas\Cache\Storage\Adapter\Exception\InvalidRedisClusterConfigurationException;
-
-final class RedisAuthProvider
-{
-    /**
-     * @psalm-return array{0: non-empty-string, 1?: non-empty-string}|null
-     * @throws InvalidRedisClusterConfigurationException
-     *
-     * Psalm cannot infer that only when user/password is not null and not an empty string, it is returned in the array
-     * @psalm-suppress MoreSpecificReturnType
-     * @psalm-suppress LessSpecificReturnStatement
-     */
-    public static function createAuthenticationObject(?string $user, ?string $password): array|null
-    {
-        if (self::isSet($password) && self::isSet($user)) {
-            return [$user, $password];
-        } elseif (self::isSet($password) && ! self::isSet($user)) {
-            return [$password];
-        } elseif (! self::isSet($password) && self::isSet($user)) {
-            throw InvalidRedisClusterConfigurationException::fromMissingRequiredPassword();
-        }
-        return null;
-    }
-
-    private static function isSet(?string $value): bool
-    {
-        return $value !== null && $value !== '';
-    }
-}

[robin-brabants]

I've refactored the authentication helper a bit but unfortunately, I am not allowed to push to your forks branch. Feel free to apply this patch to your branch and let me know what you think.

@boesing
I really like the changes you proposed! I committed them 'as is' under this commit.

I would propose still a few changes to this refactoring which I committed in this commit.
If you like the changes in this commit, I could merge them together. Just a few notes about these changes:

• I test the RedisAuthenticationInfo class now for both RedisOptions and RedisClusterOptions, also cases with username but not password provided are added to the dataProvider (we are not throwing exceptions anymore but still good to test expected behavior)
• I attempted to remove some duplicated code in RedisAuthenticationInfo

[boesing]

LGTM, I won't merge this as of now since its late here.
If some1 else of the TSC has time to merge and release this, that would work for me as well but if its still open this weekend, I will release it 👍🏼

[robin-brabants]

LGTM, I won't merge this as of now since its late here. If some1 else of the TSC has time to merge and release this, that would work for me as well but if its still open this weekend, I will release it 👍🏼

Thanks a lot again for your time and effort reviewing this PR! This was very helpful to me and I really appreciated this.

[robin-brabants]

@boesing
Could you maybe merge the PR?

[robin-brabants]

thanks a lot @boesing !!