Skip to content

Add ability to reset password in JedisFactory for JedisPool and JedisSentinelPool #2314

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

Closed
wants to merge 5 commits into from
Closed

Add ability to reset password in JedisFactory for JedisPool and JedisSentinelPool #2314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e4877e9
Add ability to reset password in JedisFactory for JedisPool and
Sep 13, 2017
0cbcf45
Modify JedisFactory tests and support ACL
sazzad16 Dec 13, 2020
e1e40c5
reduce new public
sazzad16 Dec 14, 2020
1de4e0f
volatile password instead of AtomicReference
sazzad16 Dec 14, 2020
ce6f7db
apply acl tests
sazzad16 Dec 28, 2020
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
36 changes: 25 additions & 11 deletions src/main/java/redis/clients/jedis/JedisFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,50 +18,50 @@
/**
* PoolableObjectFactory custom impl.
*/
class JedisFactory implements PooledObjectFactory<Jedis> {
public class JedisFactory implements PooledObjectFactory<Jedis> {
private final AtomicReference<HostAndPort> hostAndPort = new AtomicReference<>();
private final int connectionTimeout;
private final int soTimeout;
private final int infiniteSoTimeout;
private final String user;
private final String password;
private volatile String password;
private final int database;
private final String clientName;
private final boolean ssl;
private final SSLSocketFactory sslSocketFactory;
private final SSLParameters sslParameters;
private final HostnameVerifier hostnameVerifier;

JedisFactory(final String host, final int port, final int connectionTimeout,
protected JedisFactory(final String host, final int port, final int connectionTimeout,
final int soTimeout, final String password, final int database, final String clientName) {
this(host, port, connectionTimeout, soTimeout, password, database, clientName, false, null, null, null);
}

JedisFactory(final String host, final int port, final int connectionTimeout,
protected JedisFactory(final String host, final int port, final int connectionTimeout,
final int soTimeout, final String user, final String password, final int database, final String clientName) {
this(host, port, connectionTimeout, soTimeout, 0, user, password, database, clientName);
}

JedisFactory(final String host, final int port, final int connectionTimeout, final int soTimeout,
protected JedisFactory(final String host, final int port, final int connectionTimeout, final int soTimeout,
final int infiniteSoTimeout, final String user, final String password, final int database, final String clientName) {
this(host, port, connectionTimeout, soTimeout, infiniteSoTimeout, user, password, database, clientName, false, null, null, null);
}

JedisFactory(final String host, final int port, final int connectionTimeout,
protected JedisFactory(final String host, final int port, final int connectionTimeout,
final int soTimeout, final String password, final int database, final String clientName,
final boolean ssl, final SSLSocketFactory sslSocketFactory, final SSLParameters sslParameters,
final HostnameVerifier hostnameVerifier) {
this(host, port, connectionTimeout, soTimeout, null, password, database, clientName, ssl, sslSocketFactory, sslParameters, hostnameVerifier);
}

JedisFactory(final String host, final int port, final int connectionTimeout,
protected JedisFactory(final String host, final int port, final int connectionTimeout,
final int soTimeout, final String user, final String password, final int database, final String clientName,
final boolean ssl, final SSLSocketFactory sslSocketFactory, final SSLParameters sslParameters,
final HostnameVerifier hostnameVerifier) {
this(host, port, connectionTimeout, soTimeout, 0, user, password, database, clientName, ssl, sslSocketFactory, sslParameters, hostnameVerifier);
}

JedisFactory(final String host, final int port, final int connectionTimeout, final int soTimeout,
protected JedisFactory(final String host, final int port, final int connectionTimeout, final int soTimeout,
final int infiniteSoTimeout, final String user, final String password, final int database,
final String clientName, final boolean ssl, final SSLSocketFactory sslSocketFactory,
final SSLParameters sslParameters, final HostnameVerifier hostnameVerifier) {
Expand All @@ -79,18 +79,18 @@ class JedisFactory implements PooledObjectFactory<Jedis> {
this.hostnameVerifier = hostnameVerifier;
}

JedisFactory(final URI uri, final int connectionTimeout, final int soTimeout,
protected JedisFactory(final URI uri, final int connectionTimeout, final int soTimeout,
final String clientName) {
this(uri, connectionTimeout, soTimeout, clientName, null, null, null);
}

JedisFactory(final URI uri, final int connectionTimeout, final int soTimeout,
protected JedisFactory(final URI uri, final int connectionTimeout, final int soTimeout,
final String clientName, final SSLSocketFactory sslSocketFactory,
final SSLParameters sslParameters, final HostnameVerifier hostnameVerifier) {
this(uri, connectionTimeout, soTimeout, 0, clientName, sslSocketFactory, sslParameters, hostnameVerifier);
}

JedisFactory(final URI uri, final int connectionTimeout, final int soTimeout,
protected JedisFactory(final URI uri, final int connectionTimeout, final int soTimeout,
final int infiniteSoTimeout, final String clientName, final SSLSocketFactory sslSocketFactory,
final SSLParameters sslParameters, final HostnameVerifier hostnameVerifier) {
if (!JedisURIHelper.isValid(uri)) {
Expand All @@ -116,6 +116,20 @@ public void setHostAndPort(final HostAndPort hostAndPort) {
this.hostAndPort.set(hostAndPort);
}

public void setPassword(final String password) throws IllegalArgumentException {
if (this.user != null) {
throw new IllegalArgumentException();
}
this.password = password;
}

public void setPassword(final String user, final String password) throws IllegalArgumentException {
if (!java.util.Objects.equals(this.user, user)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@sazzad16 why do we force same user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gkorland In redis-cli on Redis ACL, after doing AUTH user pass once, if we want to AUTH again, we have to do AUTH user newpass not just AUTH newpass. I tried to imitate that here.
I also thought, this would help to us to avoid any mistaken password change as a wrong password (but a valid one for other user) would break the whole pool.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a JavaDoc explaining it

throw new IllegalArgumentException();
}
this.password = password;
}

@Override
public void activateObject(PooledObject<Jedis> pooledJedis) throws Exception {
final BinaryJedis jedis = pooledJedis.getObject();
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/redis/clients/jedis/JedisPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import javax.net.ssl.SSLParameters;
import javax.net.ssl.SSLSocketFactory;

import org.apache.commons.pool2.PooledObjectFactory;
import org.apache.commons.pool2.impl.GenericObjectPool;
import org.apache.commons.pool2.impl.GenericObjectPoolConfig;

Expand Down Expand Up @@ -325,6 +326,10 @@ public JedisPool(final GenericObjectPoolConfig poolConfig, final URI uri,
sslSocketFactory, sslParameters, hostnameVerifier));
}

public JedisPool(GenericObjectPoolConfig poolConfig, PooledObjectFactory<Jedis> factory) {
super(poolConfig, factory);
}

@Override
public Jedis getResource() {
Jedis jedis = super.getResource();
Expand Down
21 changes: 21 additions & 0 deletions src/main/java/redis/clients/jedis/JedisSentinelPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,27 @@ public JedisSentinelPool(String masterName, Set<String> sentinels,
initPool(master);
}

public JedisSentinelPool(String masterName, Set<String> sentinels, final GenericObjectPoolConfig poolConfig, final JedisFactory factory) {
this.poolConfig = poolConfig;
this.connectionTimeout = Protocol.DEFAULT_TIMEOUT;
this.soTimeout = Protocol.DEFAULT_TIMEOUT;
this.infiniteSoTimeout = 0;
this.user = null;
this.password = null;
this.database = Protocol.DEFAULT_DATABASE;
this.clientName = null;
this.sentinelConnectionTimeout = Protocol.DEFAULT_TIMEOUT;
this.sentinelSoTimeout = Protocol.DEFAULT_TIMEOUT;
this.sentinelUser = null;
this.sentinelPassword = null;
this.sentinelClientName = null;
this.factory = factory;

HostAndPort master = initSentinels(sentinels, masterName);
initPool(poolConfig, factory);
Copy link
Contributor

Choose a reason for hiding this comment

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

@sazzad16 perhaps I get it wrong, but do you need to call initPool twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gkorland initPool(poolConfig, factory); was usually get called within initPool(master); (Line 219). But as we are doing this.factory = factory; (Line 192) now, the if (factory == null) check (Line 216) would not let initPool(poolConfig, factory); to happen at usual place. So we're doing it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please at least leave a comment in the code explaining this double call.

initPool(master);
}

@Override
public void destroy() {
for (MasterListener m : masterListeners) {
Expand Down
54 changes: 54 additions & 0 deletions src/test/java/redis/clients/jedis/tests/JedisPoolTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

Expand All @@ -16,10 +17,12 @@

import redis.clients.jedis.HostAndPort;
import redis.clients.jedis.Jedis;
import redis.clients.jedis.JedisFactory;
import redis.clients.jedis.JedisPool;
import redis.clients.jedis.JedisPoolConfig;
import redis.clients.jedis.Transaction;
import redis.clients.jedis.exceptions.InvalidURIException;
import redis.clients.jedis.exceptions.JedisConnectionException;
import redis.clients.jedis.exceptions.JedisExhaustedPoolException;

public class JedisPoolTest {
Expand Down Expand Up @@ -382,4 +385,55 @@ private int getClientCount(final String clientList) {
return clientList.split("\n").length;
}

@Test
public void testResetInvalidPassword() {
JedisFactory factory = new JedisFactory(hnp.getHost(), hnp.getPort(), 2000, 2000,
"foobared", 0, "my_shiny_client_name") { };

try (JedisPool pool = new JedisPool(new JedisPoolConfig(), factory)) {
Jedis obj1;
try (Jedis obj11 = pool.getResource()) {
obj1 = obj11;
obj11.set("foo", "bar");
assertEquals("bar", obj11.get("foo"));
assertEquals(1, pool.getNumActive());
}
assertEquals(0, pool.getNumActive());
Jedis obj12 = pool.getResource();
assertSame(obj1, obj12);
assertEquals(1, pool.getNumActive());

factory.setPassword("wrong password");
try (Jedis obj2 = pool.getResource()) {
fail("Should not get resource from pool");
} catch (JedisConnectionException e) { }
assertEquals(1, pool.getNumActive());
obj12.close();
assertEquals(0, pool.getNumActive());
}
}

@Test
public void testResetValidPassword() {
JedisFactory factory = new JedisFactory(hnp.getHost(), hnp.getPort(), 2000, 2000,
"bad password", 0, "my_shiny_client_name") { };

try (JedisPool pool = new JedisPool(new JedisPoolConfig(), factory)) {
try (Jedis obj1 = pool.getResource()) {
fail("Should not get resource from pool");
} catch (JedisConnectionException e) { }
assertEquals(0, pool.getNumActive());

try {
factory.setPassword("default", "foobared");
fail();
} catch (IllegalArgumentException e) { }

factory.setPassword("foobared");
try (Jedis obj2 = pool.getResource()) {
obj2.set("foo", "bar");
assertEquals("bar", obj2.get("foo"));
}
}
}
}
54 changes: 54 additions & 0 deletions src/test/java/redis/clients/jedis/tests/JedisPoolWithCompleteCredentialsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

Expand All @@ -13,10 +14,12 @@

import redis.clients.jedis.HostAndPort;
import redis.clients.jedis.Jedis;
import redis.clients.jedis.JedisFactory;
import redis.clients.jedis.JedisPool;
import redis.clients.jedis.JedisPoolConfig;
import redis.clients.jedis.Protocol;
import redis.clients.jedis.exceptions.InvalidURIException;
import redis.clients.jedis.exceptions.JedisConnectionException;
import redis.clients.jedis.exceptions.JedisExhaustedPoolException;
import redis.clients.jedis.tests.utils.RedisVersionUtil;

Expand Down Expand Up @@ -265,4 +268,55 @@ private int getClientCount(final String clientList) {
return clientList.split("\n").length;
}

@Test
public void testResetInvalidPassword() {
JedisFactory factory = new JedisFactory(hnp.getHost(), hnp.getPort(), 2000, 2000,
"acljedis", "fizzbuzz", 0, "my_shiny_client_name") { };

try (JedisPool pool = new JedisPool(new JedisPoolConfig(), factory)) {
Jedis obj1;
try (Jedis obj11 = pool.getResource()) {
obj1 = obj11;
obj11.set("foo", "bar");
assertEquals("bar", obj11.get("foo"));
assertEquals(1, pool.getNumActive());
}
assertEquals(0, pool.getNumActive());
Jedis obj12 = pool.getResource();
assertSame(obj1, obj12);
assertEquals(1, pool.getNumActive());

factory.setPassword("acljedis", "wrong password");
try (Jedis obj2 = pool.getResource()) {
fail("Should not get resource from pool");
} catch (JedisConnectionException jce) { }
assertEquals(1, pool.getNumActive());
obj12.close();
assertEquals(0, pool.getNumActive());
}
}

@Test
public void testResetValidPassword() {
JedisFactory factory = new JedisFactory(hnp.getHost(), hnp.getPort(), 2000, 2000,
"acljedis", "bad password", 0, "my_shiny_client_name") { };

try (JedisPool pool = new JedisPool(new JedisPoolConfig(), factory)) {
try (Jedis obj1 = pool.getResource()) {
fail("Should not get resource from pool");
} catch (JedisConnectionException e) { }
assertEquals(0, pool.getNumActive());

try {
factory.setPassword("fizzbuzz");
fail();
} catch (IllegalArgumentException e) { }

factory.setPassword("acljedis", "fizzbuzz");
try (Jedis obj2 = pool.getResource()) {
obj2.set("foo", "bar");
assertEquals("bar", obj2.get("foo"));
}
}
}
}
42 changes: 42 additions & 0 deletions src/test/java/redis/clients/jedis/tests/JedisSentinelPoolTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.fail;

import java.util.HashSet;
import java.util.Set;
Expand All @@ -18,6 +19,8 @@

import redis.clients.jedis.HostAndPort;
import redis.clients.jedis.Jedis;
import redis.clients.jedis.JedisFactory;
import redis.clients.jedis.JedisPoolConfig;
import redis.clients.jedis.JedisSentinelPool;
import redis.clients.jedis.Transaction;
import redis.clients.jedis.exceptions.JedisConnectionException;
Expand Down Expand Up @@ -173,6 +176,45 @@ public void customClientName() {

assertTrue(pool.isClosed());
}

@Test
public void testResetInvalidPassword() {
JedisFactory factory = new JedisFactory(null, 0, 2000, 2000, "foobared", 0, "my_shiny_client_name") { };

try (JedisSentinelPool pool = new JedisSentinelPool(MASTER_NAME, sentinels, new JedisPoolConfig(), factory)) {
Jedis obj1;
try (Jedis obj11 = pool.getResource()) {
obj1 = obj11;
obj11.set("foo", "bar");
assertEquals("bar", obj11.get("foo"));
}
Jedis obj12 = pool.getResource();
assertSame(obj1, obj12);

factory.setPassword("wrong password");
try (Jedis obj2 = pool.getResource()) {
fail("Should not get resource from pool");
} catch (JedisConnectionException e) { }
obj12.close();
}
}

@Test
public void testResetValidPassword() {
JedisFactory factory = new JedisFactory(null, 0, 2000, 2000, "wrong password", 0, "my_shiny_client_name") { };

try (JedisSentinelPool pool = new JedisSentinelPool(MASTER_NAME, sentinels, new JedisPoolConfig(), factory)) {
try (Jedis obj1 = pool.getResource()) {
fail("Should not get resource from pool");
} catch (JedisConnectionException e) { }

factory.setPassword("foobared");
try (Jedis obj2 = pool.getResource()) {
obj2.set("foo", "bar");
assertEquals("bar", obj2.get("foo"));
}
}
}

@Test
public void ensureSafeTwiceFailover() throws InterruptedException {
Expand Down