luca3m / redis3m

A C++ Redis client
Apache License 2.0
189 stars 78 forks source link

Are there plans to support the AUTH command? #30

Closed rbraud closed 9 years ago

rbraud commented 9 years ago

We're using a redis installation that requires authentication but it seems there is no support for authentication in the current version. Is this planned?

luca3m commented 9 years ago

You can authenticate using AUTH command:

connection::ptr_t conn = connection::create();
reply auth_r = conn->run(command("AUTH") << "password);
if(auth_r.str() == "OK")
{
    conn->run(command("SET") << "foo" << "bar" );
    reply r = conn->run(command("GET") << "foo" );        
    std::cout << "FOO is: " << r.str() << std::endl;
}

Or you are talking about connection_pool and simple_pool classes?

rbraud commented 9 years ago

Sorry, I was talking about connection_pool specifically.

luca3m commented 9 years ago

You can implement it easily as I did with database here. So use the password to authenticate or throw an exception. It should be straightforward to do it.

After, if you would like to open a pull request, I'll be happy to merge it.

rbraud commented 9 years ago

Unfortunately it can't be done quite like that, because in create_master_connection()/create_slave_connection() the authentication command needs to happen before get_role() is called. Then there is also the question of whether the authentication needs to be done for both masters/slaves and sentinels, and whether the two should support different auth params too. What do you think? I can open a PR once we agree on the features you want to support.

luca3m commented 9 years ago

Yes you are right, authentication must be done on those functions.

I think authentication support for master/slaves should be enough. If you need support for authentication for sentinels, you can add it too, with different passwords. They can be implemented as setters on connection_pool, like set_password and set_sentinel_password. Current exception handling should work with this kind of stuff too, just throwing them on the place where this code will be added.

rbraud commented 9 years ago

Just opened a PR. Let me know what you think. I ended up adding a new exception type that is thrown if auth fails. On another note - if auth is required but not provided, the get_role method should probably throw an exception as well. When I was originally testing this without auth, I just got a message that the role didn't match what was expected, which wasn't the actual problem I was facing.

luca3m commented 9 years ago

I've merged it, thanks!

Yes you're right, get_role should throw some exception to say that it can't get the role.