Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add user option to Redis adapter to authenticate with a specific user #92

Merged
merged 17 commits into from
May 21, 2024

Conversation

vcg-development
Copy link

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

Description

Description
This PR is adding the possibility to authenticate with Redis with a custom username. The ACL feature is available in Redis since Version 6.
The username can be passed as an resource option.

Signed-off-by: Christian Braun <christian.braun@vogel.de>
Copy link
Member

@boesing boesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late feedback, I didn't felt like having a look into it.
However, thanks for this contribution. I wonder if it is also available for RedisCluster but I do not have a problem if that is a Redis-only feature for now, can be implemented for RedisCluster once requested.

I do have some nitpicks and some change requests.

  • especially the native return types and argument types should be added
  • we should have a test which verifies that the user can be extracted from the server URI

I think its time to have a proper resource array shape declared in the RedisResourceManager so that we do not have that weird array with "whatever" array keys in it. Once I find some time, I might implement that myself and push it to this patch - if you think you want to that on your own, I'd be happy to review that change again.

We recently introduced SSLContextArrayShape in SslContext. You can take that as an example on how to declare array shapes if you do not have an idea on how that might work.

Once we defined the RedisResourceArrayShape, we can use that for RedisResourceManager::$resources. That will most probably remove a bunch of baseline entries and errors within the RedisResourceManager so that would be a huge improvement.

src/RedisOptions.php Outdated Show resolved Hide resolved
src/RedisOptions.php Outdated Show resolved Hide resolved
src/RedisResourceManager.php Outdated Show resolved Hide resolved
src/RedisResourceManager.php Show resolved Hide resolved
src/RedisResourceManager.php Outdated Show resolved Hide resolved
src/RedisResourceManager.php Show resolved Hide resolved
src/RedisResourceManager.php Outdated Show resolved Hide resolved
test/unit/RedisResourceManagerTest.php Show resolved Hide resolved
test/unit/RedisResourceManagerTest.php Show resolved Hide resolved
cbraunVBM added 5 commits May 7, 2024 10:10
Signed-off-by: Christian Braun <christian.braun@vogel.de>
Signed-off-by: Christian Braun <christian.braun@vogel.de>
Signed-off-by: Christian Braun <christian.braun@vogel.de>
Signed-off-by: Christian Braun <christian.braun@vogel.de>
Signed-off-by: Christian Braun <christian.braun@vogel.de>
@vcg-development
Copy link
Author

Thank you very much for the detailed review. I changed the code according to your comments.

I also added the suggested ResourceArrayShape. I`d be happy, if have another look at my changes.

Copy link
Member

@boesing boesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for updating and providing proper type-hints for the new methods!
I put some suggestions and comments to your latest changes regarding the array shape. If you apply these changes, I guess psalm will stop reporting some errors.

You might want to have a look on these psalm issues as they might tell you why some types may probably not correct 👍🏼

If you need help, just let me know.

src/RedisResourceManager.php Outdated Show resolved Hide resolved
src/RedisResourceManager.php Outdated Show resolved Hide resolved
src/RedisResourceManager.php Outdated Show resolved Hide resolved
src/RedisResourceManager.php Outdated Show resolved Hide resolved
src/RedisResourceManager.php Outdated Show resolved Hide resolved
src/RedisResourceManager.php Show resolved Hide resolved
cbraunVBM added 2 commits May 21, 2024 10:14
Signed-off-by: Christian Braun <christian.braun@vogel.de>
Signed-off-by: Christian Braun <christian.braun@vogel.de>
@vcg-development
Copy link
Author

Thank you again for having a deep look at my commits again. The types declared in the array shape and related return types should be correct now.
I thing the failing psalm checks come from the array_merge in line 416. At the moment I am not sure how this could be fixed, so that they will not fail anymore.

Also I think the Assertment in Line 177 is not necessary anymore, because the instance is already checked in line 169.

Due to the way how this class worked before, we make an assumption that a provided `iterable` will contain at least those keys we are supporting.

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
Copy link
Member

boesing commented May 21, 2024

Yes you are right, one of the issues were due to the array_merge (psalm has issues with that while array_replace does work. In this case, array_replace should suffice and thus I changed the underlying code.

I also removed the fixed issues with psalm from the baseline by updating the baseline using psalm --update-baseline. That will automatically remove all fixed issues which were fixed by this PR.

I will have a closer look on the integration tests, it seems that the password test is actually introducing problems when used on a server without a configured password.

boesing added 6 commits May 21, 2024 20:28
Due to refactoring, the property reference was removed but the resource was only re-written to the propery cache when the version was not available. This is fixed with this commit.

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
To have the same functionality as in other methods (which provide fluent interface), we should use early return to prevent unsetting `initialized` logic.

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
…ase selection was executed

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
…rage` property anymore

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
Copy link
Member

boesing commented May 21, 2024

Okay, that was a tough one. I was wondering why stuff started to went wrong after my changes.
I found out that in one of my changes, I removed the property reference which had side-effects in some tests.

Another good reason on why having these property references removed from our code.
I've created #93 so that we can tackle further refactorings in v3.0.0.

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing boesing added this to the 2.9.0 milestone May 21, 2024
@boesing boesing changed the title Added option to set redis user name in redis options Add user option to Redis adapter to authenticate with a specific user May 21, 2024
@boesing boesing merged commit 3b234a8 into laminas:2.9.x May 21, 2024
14 of 15 checks passed
@boesing
Copy link
Member

boesing commented May 21, 2024

Thanks @vcg-development and @cbraunVBM!
I will create an issue in laminas-cache so that we can document the new option as that has to be done there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants