Skip to content

Commit d0624db

Browse files
Vladimir Kotalahornace
Vladimir Kotal
authored and
ahornace
committed
fix LDAP timeout handling for LdapCtx
also adds read timeout
1 parent df8ef63 commit d0624db

File tree

5 files changed

+84
-22
lines changed

5 files changed

+84
-22
lines changed

‎plugins/src/main/java/opengrok/auth/plugin/configuration/Configuration.java

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
*/
1919

2020
/*
21-
* Copyright (c) 2016, 2017, Oracle and/or its affiliates. All rights reserved.
21+
* Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.
2222
*/
2323
package opengrok.auth.plugin.configuration;
2424

@@ -39,6 +39,9 @@
3939
import opengrok.auth.plugin.ldap.LdapServer;
4040
import opengrok.auth.plugin.util.WebHooks;
4141

42+
/**
43+
* Encapsulates configuration for LDAP plugins.
44+
*/
4245
public class Configuration implements Serializable {
4346

4447
private static final long serialVersionUID = -1;
@@ -49,6 +52,7 @@ public class Configuration implements Serializable {
4952
private WebHooks webHooks;
5053
private int searchTimeout;
5154
private int connectTimeout;
55+
private int readTimeout;
5256
private int countLimit;
5357

5458
public void setServers(List<LdapServer> servers) {
@@ -91,6 +95,14 @@ public void setConnectTimeout(int timeout) {
9195
this.connectTimeout = timeout;
9296
}
9397

98+
public int getReadTimeout() {
99+
return this.readTimeout;
100+
}
101+
102+
public void setReadTimeout(int timeout) {
103+
this.readTimeout = timeout;
104+
}
105+
94106
public int getCountLimit() {
95107
return this.countLimit;
96108
}

‎plugins/src/main/java/opengrok/auth/plugin/ldap/LdapFacade.java

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ private void addAttrToMap(Map<String, Set<String>> map, Attribute attr) throws N
179179
}
180180

181181
public LdapFacade(Configuration cfg) {
182-
setServers(cfg.getServers(), cfg.getConnectTimeout());
182+
setServers(cfg.getServers(), cfg.getConnectTimeout(), cfg.getReadTimeout());
183183
setInterval(cfg.getInterval());
184184
setSearchBase(cfg.getSearchBase());
185185
setWebHooks(cfg.getWebHooks());
@@ -219,12 +219,15 @@ public List<LdapServer> getServers() {
219219
return servers;
220220
}
221221

222-
public LdapFacade setServers(List<LdapServer> servers, int timeout) {
222+
public LdapFacade setServers(List<LdapServer> servers, int connectTimeout, int readTimeout) {
223223
this.servers = servers;
224-
// Inherit connect timeout from server pool configuration.
224+
// Inherit timeout values from server pool configuration.
225225
for (LdapServer server : servers) {
226-
if (server.getConnectTimeout() == 0 && timeout != 0) {
227-
server.setConnectTimeout(timeout);
226+
if (server.getConnectTimeout() == 0 && connectTimeout != 0) {
227+
server.setConnectTimeout(connectTimeout);
228+
}
229+
if (server.getReadTimeout() == 0 && readTimeout != 0) {
230+
server.setReadTimeout(readTimeout);
228231
}
229232
}
230233
return this;

‎plugins/src/main/java/opengrok/auth/plugin/ldap/LdapServer.java

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
*/
1919

2020
/*
21-
* Copyright (c) 2016, 2017, Oracle and/or its affiliates. All rights reserved.
21+
* Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.
2222
*/
2323
package opengrok.auth.plugin.ldap;
2424

@@ -48,17 +48,20 @@ public class LdapServer implements Serializable {
4848

4949
private static final Logger LOGGER = Logger.getLogger(LdapServer.class.getName());
5050

51-
private static final String LDAP_TIMEOUT_PARAMETER = "com.sun.jndi.ldap.connect.connectTimeout";
51+
private static final String LDAP_CONNECT_TIMEOUT_PARAMETER = "com.sun.jndi.ldap.connect.timeout";
52+
private static final String LDAP_READ_TIMEOUT_PARAMETER = "com.sun.jndi.ldap.read.timeout";
5253
private static final String LDAP_CONTEXT_FACTORY = "com.sun.jndi.ldap.LdapCtxFactory";
53-
/**
54-
* Default connectTimeout for connecting.
55-
*/
56-
private static final int LDAP_CONNECT_TIMEOUT = 5000; // ms
54+
55+
// default connectTimeout value in milliseconds
56+
private static final int LDAP_CONNECT_TIMEOUT = 5000;
57+
// default readTimeout value in milliseconds
58+
private static final int LDAP_READ_TIMEOUT = 3000;
5759

5860
private String url;
5961
private String username;
6062
private String password;
6163
private int connectTimeout;
64+
private int readTimeout;
6265
private int interval = 10 * 1000;
6366

6467
private Hashtable<String, String> env;
@@ -121,6 +124,15 @@ public LdapServer setConnectTimeout(int connectTimeout) {
121124
return this;
122125
}
123126

127+
public int getReadTimeout() {
128+
return readTimeout;
129+
}
130+
131+
public LdapServer setReadTimeout(int readTimeout) {
132+
this.readTimeout = readTimeout;
133+
return this;
134+
}
135+
124136
public int getInterval() {
125137
return interval;
126138
}
@@ -248,7 +260,10 @@ private synchronized LdapContext connect() {
248260
env.put(Context.SECURITY_CREDENTIALS, this.password);
249261
}
250262
if (this.connectTimeout > 0) {
251-
env.put(LDAP_TIMEOUT_PARAMETER, Integer.toString(this.connectTimeout));
263+
env.put(LDAP_CONNECT_TIMEOUT_PARAMETER, Integer.toString(this.connectTimeout));
264+
}
265+
if (this.readTimeout > 0) {
266+
env.put(LDAP_READ_TIMEOUT_PARAMETER, Integer.toString(this.readTimeout));
252267
}
253268

254269
try {
@@ -341,7 +356,8 @@ private static Hashtable<String, String> prepareEnv() {
341356
Hashtable<String, String> e = new Hashtable<String, String>();
342357

343358
e.put(Context.INITIAL_CONTEXT_FACTORY, LDAP_CONTEXT_FACTORY);
344-
e.put(LDAP_TIMEOUT_PARAMETER, Integer.toString(LDAP_CONNECT_TIMEOUT));
359+
e.put(LDAP_CONNECT_TIMEOUT_PARAMETER, Integer.toString(LDAP_CONNECT_TIMEOUT));
360+
e.put(LDAP_READ_TIMEOUT_PARAMETER, Integer.toString(LDAP_READ_TIMEOUT));
345361

346362
return e;
347363
}
@@ -353,12 +369,16 @@ public String toString() {
353369
sb.append(getUrl());
354370

355371
if (getConnectTimeout() > 0) {
356-
sb.append(" timeout: ");
372+
sb.append(", connect timeout: ");
357373
sb.append(getConnectTimeout());
358374
}
375+
if (getReadTimeout() > 0) {
376+
sb.append(", read timeout: ");
377+
sb.append(getReadTimeout());
378+
}
359379

360380
if (getUsername() != null && !getUsername().isEmpty()) {
361-
sb.append(" username: ");
381+
sb.append(", username: ");
362382
sb.append(getUsername());
363383
}
364384

‎plugins/src/test/java/opengrok/auth/plugin/LdapServerTest.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,4 +107,13 @@ public void testEmptyAddressArray() throws UnknownHostException {
107107
Mockito.when(serverSpy.getAddresses(any())).thenReturn(new InetAddress[]{});
108108
assertFalse(serverSpy.isReachable());
109109
}
110+
111+
@Test
112+
public void testToString() {
113+
LdapServer server = new LdapServer("ldaps://foo.bar.com", "foo", "bar");
114+
server.setConnectTimeout(2000);
115+
server.setReadTimeout(1000);
116+
assertEquals("ldaps://foo.bar.com, connect timeout: 2000, read timeout: 1000, username: foo",
117+
server.toString());
118+
}
110119
}

‎plugins/src/test/java/opengrok/auth/plugin/ldap/LdapFacadeTest.java

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import java.util.ArrayList;
1515
import java.util.Arrays;
1616
import java.util.Collections;
17+
import java.util.stream.Collectors;
1718

1819
import static org.junit.Assert.assertEquals;
1920
import static org.junit.Assert.assertTrue;
@@ -35,14 +36,31 @@ public void testSearchControlsConfig() {
3536
assertEquals(countLimit, controls.getCountLimit());
3637
}
3738

39+
private LdapServer getSpyLdapServer(String name) throws UnknownHostException {
40+
LdapServer server1 = new LdapServer(name);
41+
LdapServer serverSpy1 = Mockito.spy(server1);
42+
Mockito.when(serverSpy1.getAddresses(any())).thenReturn(new InetAddress[]{InetAddress.getLocalHost()});
43+
Mockito.when(serverSpy1.isWorking()).thenReturn(true);
44+
return serverSpy1;
45+
}
46+
3847
@Test
39-
public void testConnectTimeoutInheritance() {
48+
public void testConnectTimeoutInheritance() throws UnknownHostException {
4049
Configuration config = new Configuration();
41-
config.setServers(Collections.singletonList(new LdapServer("http://foo.bar")));
42-
int timeoutValue = 42;
43-
config.setConnectTimeout(timeoutValue);
50+
51+
LdapServer[] servers = {getSpyLdapServer("ldap://foo.com"), getSpyLdapServer("ldap://bar.com")};
52+
config.setServers(Arrays.asList(servers));
53+
54+
int connectTimeoutValue = 42;
55+
config.setConnectTimeout(connectTimeoutValue);
56+
int readTimeoutValue = 24;
57+
config.setReadTimeout(readTimeoutValue);
58+
4459
LdapFacade facade = new LdapFacade(config);
45-
assertTrue(facade.getServers().stream().anyMatch(s -> s.getConnectTimeout() == timeoutValue));
60+
assertEquals(Collections.singleton(connectTimeoutValue),
61+
facade.getServers().stream().map(s -> s.getConnectTimeout()).collect(Collectors.toSet()));
62+
assertEquals(Collections.singleton(readTimeoutValue),
63+
facade.getServers().stream().map(s -> s.getReadTimeout()).collect(Collectors.toSet()));
4664
}
4765

4866
@Test
@@ -74,7 +92,7 @@ public void testToStringPositive() throws UnknownHostException {
7492
int timeoutValue = 3;
7593
config.setConnectTimeout(timeoutValue);
7694
LdapFacade facade = new LdapFacade(config);
77-
assertEquals("{server=ldap://foo.com timeout: 3, searchBase=dc=foo,dc=com}",
95+
assertEquals("{server=ldap://foo.com, connect timeout: 3, searchBase=dc=foo,dc=com}",
7896
facade.toString());
7997
}
8098

0 commit comments

Comments
 (0)