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.

Sign up for GitHub

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

fix(auth): selecting database before auth caused error #85

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

fix(auth): selecting database before auth caused error #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions lib/Redis.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ public static function prefix($namespace)
* DSN-supplied value will be used instead and this parameter is ignored.
* @param object $client Optional Credis_Cluster or Credis_Client instance instantiated by you
*/
public function __construct($server, $database = null, $client = null)
public function __construct($server, $database = null, $client = null, $auth = null)
{
try {
if (is_object($client)) {
Expand All @@ -172,7 +172,7 @@ public function __construct($server, $database = null, $client = null)
$this->driver = new Credis_Client($host, $port, $timeout, $persistent);
$this->driver->setMaxConnectRetries($maxRetries);
if ($password) {
$this->driver->auth($password);
$auth = $password;
}

// If we have found a database in our DSN, use it instead of the `$database`
Expand All @@ -182,6 +182,10 @@ public function __construct($server, $database = null, $client = null)
}
}

if ($auth !== null) {
$this->driver->auth($auth);
}

if ($database !== null) {
$this->driver->select($database);
Copy link
Author

Choose a reason for hiding this comment

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

This causes error if auth is not executed.

}
Expand Down
2 changes: 1 addition & 1 deletion lib/Resque.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public static function redis()
if (is_callable(self::$redisServer)) {
self::$redis = call_user_func(self::$redisServer, self::$redisDatabase);
} else {
self::$redis = new Redis(self::$redisServer, self::$redisDatabase);
self::$redis = new Redis(self::$redisServer, self::$redisDatabase, null, self::$auth);
Copy link
Author

Choose a reason for hiding this comment

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

This is passing $redisDatabase, which causes a ->select() in constructor. Line 78 passes auth after the select.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the value of $redisServer that triggers the error for you?

Copy link
Author

Choose a reason for hiding this comment

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

The value of $redisServer is 10.181.53.75. We are running a local redis server in our network - and that server has auth configured.

Copy link
Author

Choose a reason for hiding this comment

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

So when $redisServer is set to 10.181.53.75 and $redisDatabase is set to 1, the error occurs. If the $redisDatabase is 0, it does not fail, because ->select() is not ran.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use tcp://user:pass@host:port/db as a value? The user part of it is ignored, but that wouldn't be part of $auth either so that wouldn't matter

}

if (!empty(self::$auth)) {
Copy link
Author

Choose a reason for hiding this comment

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

Here, auth is passed, but it is too late.

Copy link
Member

@danhunsaker danhunsaker Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Pretty sure the if block after this one should be removed in this PR, since trying to auth twice isn't technically valid.

Expand Down