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

Only provide username for ACL if username is non-empty #281

Closed

Conversation

frwdrik
Copy link

@frwdrik frwdrik commented Apr 23, 2023

Issue: When given a URI like redis://:pass@localhost:6379-/, Carmine 3.1.0 would authenticate to Redis using only password, while Carmine 3.2.0 now passes an empty string "" as username to Redis. This is a breaking change.

Fix: Only add username to connection spec when it's parsed as a non-empty string. Another option is to supply the implicit default username that Redis uses when no username is provided.

@frwdrik frwdrik force-pushed the frwdrik-don't-supply-empty-username branch from 1e7dac1 to 312ec54 Compare April 23, 2023 14:11
@ptaoussanis ptaoussanis self-assigned this Apr 24, 2023
@ptaoussanis
Copy link
Member

@frwdrik Hi Fredrik, thanks for this! And apologies for the unintended break!

This will be included in Carmine v3.3 currently scheduled for June, or sooner if I cut any hotfixes before then.

In the meantime, you can either adjust your URI or switch to using explicit conn opts to avoid the (less flexible / more fragile) parser. See also #279

Cheers :-)

@frwdrik
Copy link
Author

frwdrik commented Apr 24, 2023

Hi! Thanks for the quick response :)
Apologies for not seeing #279, I also see the recent commit on master is a fix for that issue. Looking forward to the next release!
Closing this PR now.

@frwdrik frwdrik closed this Apr 24, 2023
@ptaoussanis
Copy link
Member

Apologies for not seeing #279

No worries, it's only partly related - your particular issue still required an independent fix.

I also see the recent commit on master is a fix for that issue.

To clarify: the recent commit on master is basically your proposed fix :-)
I wouldn't have been aware of this if you hadn't raised your issue 👍

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

Successfully merging this pull request may close these issues.

2 participants