From e7c2bf25f57a29f55cd74e819cc22f04df3413c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20K=C3=B6rber?= Date: Wed, 30 Oct 2019 22:31:21 +0100 Subject: [PATCH] Add a way to change network timeouts --- .../java/org/shredzone/acme4j/Session.java | 23 ++++- .../acme4j/connector/DefaultConnection.java | 6 +- .../acme4j/connector/HttpConnector.java | 30 ++++--- .../acme4j/connector/NetworkSettings.java | 72 ++++++++++++++++ .../provider/pebble/PebbleHttpConnector.java | 8 +- .../org/shredzone/acme4j/SessionTest.java | 11 +-- .../connector/DefaultConnectionTest.java | 9 +- .../acme4j/connector/HttpConnectorTest.java | 17 ++-- .../acme4j/connector/NetworkSettingsTest.java | 83 +++++++++++++++++++ src/site/markdown/usage/session.md | 9 +- 10 files changed, 220 insertions(+), 48 deletions(-) create mode 100644 acme4j-client/src/main/java/org/shredzone/acme4j/connector/NetworkSettings.java create mode 100644 acme4j-client/src/test/java/org/shredzone/acme4j/connector/NetworkSettingsTest.java diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/Session.java b/acme4j-client/src/main/java/org/shredzone/acme4j/Session.java index cf67d34a..865c9ebc 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/Session.java +++ b/acme4j-client/src/main/java/org/shredzone/acme4j/Session.java @@ -33,6 +33,7 @@ import javax.annotation.ParametersAreNonnullByDefault; import javax.annotation.concurrent.ThreadSafe; import org.shredzone.acme4j.connector.Connection; +import org.shredzone.acme4j.connector.NetworkSettings; import org.shredzone.acme4j.connector.Resource; import org.shredzone.acme4j.exception.AcmeException; import org.shredzone.acme4j.provider.AcmeProvider; @@ -51,12 +52,12 @@ public class Session { private final AtomicReference> resourceMap = new AtomicReference<>(); private final AtomicReference metadata = new AtomicReference<>(); + private final NetworkSettings networkSettings = new NetworkSettings(); private final URI serverUri; private final AcmeProvider provider; private String nonce; private Locale locale = Locale.getDefault(); - private Proxy proxy = Proxy.NO_PROXY; protected Instant directoryCacheExpiry; /** @@ -171,17 +172,33 @@ public class Session { /** * Gets the {@link Proxy} to be used for connections. + * + * @deprecated Use {@code networkSettings().getProxy()} */ + @Deprecated public Proxy getProxy() { - return proxy; + return networkSettings.getProxy(); } /** * Sets a {@link Proxy} that is to be used for all connections. If {@code null}, * {@link Proxy#NO_PROXY} is used, which is also the default. + * + * @deprecated Use {@code networkSettings().setProxy(Proxy)} */ + @Deprecated public void setProxy(@Nullable Proxy proxy) { - this.proxy = proxy != null ? proxy : Proxy.NO_PROXY; + networkSettings.setProxy(proxy); + } + + /** + * Returns the current {@link NetworkSettings}. + * + * @return {@link NetworkSettings} + * @since 2.8 + */ + public NetworkSettings networkSettings() { + return networkSettings; } /** diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/connector/DefaultConnection.java b/acme4j-client/src/main/java/org/shredzone/acme4j/connector/DefaultConnection.java index 286ee89d..df74ca10 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/connector/DefaultConnection.java +++ b/acme4j-client/src/main/java/org/shredzone/acme4j/connector/DefaultConnection.java @@ -107,7 +107,7 @@ public class DefaultConnection implements Connection { LOG.debug("HEAD {}", newNonceUrl); - conn = httpConnector.openConnection(newNonceUrl, session.getProxy()); + conn = httpConnector.openConnection(newNonceUrl, session.networkSettings()); conn.setRequestMethod("HEAD"); conn.setRequestProperty(ACCEPT_LANGUAGE_HEADER, session.getLocale().toLanguageTag()); conn.connect(); @@ -284,7 +284,7 @@ public class DefaultConnection implements Connection { LOG.debug("GET {}", url); try { - conn = httpConnector.openConnection(url, session.getProxy()); + conn = httpConnector.openConnection(url, session.networkSettings()); conn.setRequestMethod("GET"); conn.setRequestProperty(ACCEPT_HEADER, accept); conn.setRequestProperty(ACCEPT_CHARSET_HEADER, DEFAULT_CHARSET); @@ -381,7 +381,7 @@ public class DefaultConnection implements Connection { resetNonce(session); } - conn = httpConnector.openConnection(url, session.getProxy()); + conn = httpConnector.openConnection(url, session.networkSettings()); conn.setRequestMethod("POST"); conn.setRequestProperty(ACCEPT_HEADER, accept); conn.setRequestProperty(ACCEPT_CHARSET_HEADER, DEFAULT_CHARSET); diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/connector/HttpConnector.java b/acme4j-client/src/main/java/org/shredzone/acme4j/connector/HttpConnector.java index 4bafc434..fe7215c3 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/connector/HttpConnector.java +++ b/acme4j-client/src/main/java/org/shredzone/acme4j/connector/HttpConnector.java @@ -16,10 +16,10 @@ package org.shredzone.acme4j.connector; import java.io.IOException; import java.io.InputStream; import java.net.HttpURLConnection; -import java.net.Proxy; import java.net.URL; import java.util.Properties; +import javax.annotation.OverridingMethodsMustInvokeSuper; import javax.annotation.ParametersAreNonnullByDefault; import javax.annotation.concurrent.ThreadSafe; @@ -36,7 +36,6 @@ import org.slf4j.LoggerFactory; @ThreadSafe public class HttpConnector { - private static final int TIMEOUT = 10000; private static final String USER_AGENT; static { @@ -69,28 +68,33 @@ public class HttpConnector { * * @param url * {@link URL} to connect to - * @param proxy - * {@link Proxy} to be used + * @param settings + * {@link NetworkSettings} to be used * @return {@link HttpURLConnection} connected to the {@link URL} */ - public HttpURLConnection openConnection(URL url, Proxy proxy) throws IOException { - HttpURLConnection conn = (HttpURLConnection) url.openConnection(proxy); - configure(conn); + public HttpURLConnection openConnection(URL url, NetworkSettings settings) throws IOException { + HttpURLConnection conn = (HttpURLConnection) url.openConnection(settings.getProxy()); + configure(conn, settings); return conn; } /** * Configures the new {@link HttpURLConnection}. *

- * This implementation sets reasonable timeouts, forbids caching, and sets an user - * agent. + * The {@link HttpURLConnection} is already preconfigured with a reasonable timeout, + * disabled caches and a User-Agent header. Subclasses can override this method to + * change the configuration. * * @param conn - * {@link HttpURLConnection} to configure. + * {@link HttpURLConnection} to configure. + * @param settings + * {@link NetworkSettings} with settings to be used */ - protected void configure(HttpURLConnection conn) { - conn.setConnectTimeout(TIMEOUT); - conn.setReadTimeout(TIMEOUT); + @OverridingMethodsMustInvokeSuper + protected void configure(HttpURLConnection conn, NetworkSettings settings) { + int timeout = (int) settings.getTimeout().toMillis(); + conn.setConnectTimeout(timeout); + conn.setReadTimeout(timeout); conn.setUseCaches(false); conn.setRequestProperty("User-Agent", USER_AGENT); } diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/connector/NetworkSettings.java b/acme4j-client/src/main/java/org/shredzone/acme4j/connector/NetworkSettings.java new file mode 100644 index 00000000..d8dc74b6 --- /dev/null +++ b/acme4j-client/src/main/java/org/shredzone/acme4j/connector/NetworkSettings.java @@ -0,0 +1,72 @@ +/* + * acme4j - Java ACME client + * + * Copyright (C) 2019 Richard "Shred" Körber + * http://acme4j.shredzone.org + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + */ +package org.shredzone.acme4j.connector; + +import java.net.Proxy; +import java.time.Duration; + +import javax.annotation.Nullable; +import javax.annotation.ParametersAreNonnullByDefault; + +/** + * Contains network settings to be used for network connections. + * + * @since 2.8 + */ +@ParametersAreNonnullByDefault +public class NetworkSettings { + + private Proxy proxy = Proxy.NO_PROXY; + private Duration timeout = Duration.ofSeconds(10); + + /** + * Gets the {@link Proxy} to be used for connections. + */ + public Proxy getProxy() { + return proxy; + } + + /** + * Sets a {@link Proxy} that is to be used for all connections. If {@code null}, + * {@link Proxy#NO_PROXY} is used, which is also the default. + */ + public void setProxy(@Nullable Proxy proxy) { + this.proxy = proxy != null ? proxy : Proxy.NO_PROXY; + } + + /** + * Gets the current network timeout. + */ + public Duration getTimeout() { + return timeout; + } + + /** + * Sets the network timeout to be used for connections. Defaults to 10 seconds. + * + * @param timeout + * Network timeout {@link Duration} + */ + public void setTimeout(Duration timeout) { + if (timeout == null || timeout.isNegative() || timeout.isZero()) { + throw new IllegalArgumentException("Timeout must be positive"); + } + if (timeout.toMillis() > Integer.MAX_VALUE) { + throw new IllegalArgumentException("Timeout is out of range"); + } + + this.timeout = timeout; + } + +} diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/provider/pebble/PebbleHttpConnector.java b/acme4j-client/src/main/java/org/shredzone/acme4j/provider/pebble/PebbleHttpConnector.java index cff21e12..10498090 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/provider/pebble/PebbleHttpConnector.java +++ b/acme4j-client/src/main/java/org/shredzone/acme4j/provider/pebble/PebbleHttpConnector.java @@ -16,7 +16,6 @@ package org.shredzone.acme4j.provider.pebble; import java.io.IOException; import java.io.InputStream; import java.net.HttpURLConnection; -import java.net.Proxy; import java.net.URL; import java.security.KeyManagementException; import java.security.KeyStore; @@ -32,6 +31,7 @@ import javax.net.ssl.SSLSocketFactory; import javax.net.ssl.TrustManagerFactory; import org.shredzone.acme4j.connector.HttpConnector; +import org.shredzone.acme4j.connector.NetworkSettings; /** * {@link HttpConnector} to be used for Pebble. Pebble uses a static, self signed SSL @@ -43,12 +43,12 @@ public class PebbleHttpConnector extends HttpConnector { private static SSLSocketFactory sslSocketFactory; @Override - public HttpURLConnection openConnection(URL url, Proxy proxy) throws IOException { - HttpURLConnection conn = super.openConnection(url, proxy); + public HttpURLConnection openConnection(URL url, NetworkSettings settings) throws IOException { + HttpURLConnection conn = super.openConnection(url, settings); if (conn instanceof HttpsURLConnection) { HttpsURLConnection conns = (HttpsURLConnection) conn; conns.setSSLSocketFactory(createSocketFactory()); - conns.setHostnameVerifier((hostname, session) -> true); + conns.setHostnameVerifier((h, s) -> true); } return conn; } diff --git a/acme4j-client/src/test/java/org/shredzone/acme4j/SessionTest.java b/acme4j-client/src/test/java/org/shredzone/acme4j/SessionTest.java index 346680e6..8e2f2d1d 100644 --- a/acme4j-client/src/test/java/org/shredzone/acme4j/SessionTest.java +++ b/acme4j-client/src/test/java/org/shredzone/acme4j/SessionTest.java @@ -20,9 +20,6 @@ import static org.mockito.Mockito.*; import static org.shredzone.acme4j.toolbox.TestUtils.*; import java.io.IOException; -import java.net.InetSocketAddress; -import java.net.Proxy; -import java.net.Proxy.Type; import java.net.URI; import java.net.URL; import java.security.KeyPair; @@ -96,14 +93,8 @@ public class SessionTest { session.setNonce(DUMMY_NONCE); assertThat(session.getNonce(), is(equalTo(DUMMY_NONCE))); - assertThat(session.getProxy(), is(Proxy.NO_PROXY)); - Proxy proxy = new Proxy(Type.HTTP, new InetSocketAddress("10.0.0.1", 8080)); - session.setProxy(proxy); - assertThat(session.getProxy(), is(proxy)); - session.setProxy(null); - assertThat(session.getProxy(), is(Proxy.NO_PROXY)); - assertThat(session.getServerUri(), is(serverUri)); + assertThat(session.networkSettings(), is(notNullValue())); } /** diff --git a/acme4j-client/src/test/java/org/shredzone/acme4j/connector/DefaultConnectionTest.java b/acme4j-client/src/test/java/org/shredzone/acme4j/connector/DefaultConnectionTest.java index 7c9f9d6f..0f609ba2 100644 --- a/acme4j-client/src/test/java/org/shredzone/acme4j/connector/DefaultConnectionTest.java +++ b/acme4j-client/src/test/java/org/shredzone/acme4j/connector/DefaultConnectionTest.java @@ -17,6 +17,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.hamcrest.Matchers.*; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; +import static org.mockito.Mockito.any; import static org.mockito.Mockito.*; import static org.shredzone.acme4j.toolbox.TestUtils.url; import static uk.co.datumedge.hamcrest.json.SameJSONAs.sameJSONAs; @@ -26,7 +27,6 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.OutputStreamWriter; import java.net.HttpURLConnection; -import java.net.Proxy; import java.net.URI; import java.net.URL; import java.security.KeyPair; @@ -72,6 +72,7 @@ public class DefaultConnectionTest { private URL requestUrl = TestUtils.url("http://example.com/acme/"); private URL accountUrl = TestUtils.url(TestUtils.ACCOUNT_URL); + private NetworkSettings settings = new NetworkSettings(); private HttpURLConnection mockUrlConnection; private HttpConnector mockHttpConnection; private Session session; @@ -83,7 +84,7 @@ public class DefaultConnectionTest { mockUrlConnection = mock(HttpURLConnection.class); mockHttpConnection = mock(HttpConnector.class); - when(mockHttpConnection.openConnection(requestUrl, Proxy.NO_PROXY)).thenReturn(mockUrlConnection); + when(mockHttpConnection.openConnection(same(requestUrl), any())).thenReturn(mockUrlConnection); final AcmeProvider mockProvider = mock(AcmeProvider.class); when(mockProvider.directory( @@ -163,7 +164,7 @@ public class DefaultConnectionTest { */ @Test public void testResetNonce() throws AcmeException, IOException { - when(mockHttpConnection.openConnection(new URL("https://example.com/acme/new-nonce"), Proxy.NO_PROXY)) + when(mockHttpConnection.openConnection(eq(new URL("https://example.com/acme/new-nonce")), any())) .thenReturn(mockUrlConnection); when(mockUrlConnection.getResponseCode()) .thenReturn(HttpURLConnection.HTTP_NO_CONTENT); @@ -955,7 +956,7 @@ public class DefaultConnectionTest { */ @Test(expected = AcmeException.class) public void testSendSignedRequestNoNonce() throws Exception { - when(mockHttpConnection.openConnection(new URL("https://example.com/acme/new-nonce"), Proxy.NO_PROXY)) + when(mockHttpConnection.openConnection(eq(new URL("https://example.com/acme/new-nonce")), any())) .thenReturn(mockUrlConnection); when(mockUrlConnection.getResponseCode()) .thenReturn(HttpURLConnection.HTTP_NOT_FOUND); diff --git a/acme4j-client/src/test/java/org/shredzone/acme4j/connector/HttpConnectorTest.java b/acme4j-client/src/test/java/org/shredzone/acme4j/connector/HttpConnectorTest.java index 202cb223..e28cec04 100644 --- a/acme4j-client/src/test/java/org/shredzone/acme4j/connector/HttpConnectorTest.java +++ b/acme4j-client/src/test/java/org/shredzone/acme4j/connector/HttpConnectorTest.java @@ -15,14 +15,13 @@ package org.shredzone.acme4j.connector; import static org.hamcrest.Matchers.*; import static org.junit.Assert.assertThat; -import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import java.io.IOException; import java.net.HttpURLConnection; -import java.net.Proxy; import java.net.URL; +import java.time.Duration; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -38,14 +37,17 @@ public class HttpConnectorTest { * This is just a mock to check that the parameters are properly set. */ @Test - public void testMockOpenConnection() { + public void testMockOpenConnection() throws IOException { + NetworkSettings settings = new NetworkSettings(); + settings.setTimeout(Duration.ofSeconds(50)); + HttpURLConnection conn = mock(HttpURLConnection.class); HttpConnector connector = new HttpConnector(); - connector.configure(conn); + connector.configure(conn, settings); - verify(conn).setConnectTimeout(anyInt()); - verify(conn).setReadTimeout(anyInt()); + verify(conn).setConnectTimeout(50000); + verify(conn).setReadTimeout(50000); verify(conn).setUseCaches(false); verify(conn).setRequestProperty("User-Agent", HttpConnector.defaultUserAgent()); } @@ -59,8 +61,9 @@ public class HttpConnectorTest { @Test @Category(HttpURLConnection.class) public void testOpenConnection() throws IOException { + NetworkSettings settings = new NetworkSettings(); HttpConnector connector = new HttpConnector(); - HttpURLConnection conn = connector.openConnection(new URL("http://example.com"), Proxy.NO_PROXY); + HttpURLConnection conn = connector.openConnection(new URL("http://example.com"), settings); assertThat(conn, not(nullValue())); conn.connect(); assertThat(conn.getResponseCode(), is(HttpURLConnection.HTTP_OK)); diff --git a/acme4j-client/src/test/java/org/shredzone/acme4j/connector/NetworkSettingsTest.java b/acme4j-client/src/test/java/org/shredzone/acme4j/connector/NetworkSettingsTest.java new file mode 100644 index 00000000..03d92c55 --- /dev/null +++ b/acme4j-client/src/test/java/org/shredzone/acme4j/connector/NetworkSettingsTest.java @@ -0,0 +1,83 @@ +/* + * acme4j - Java ACME client + * + * Copyright (C) 2019 Richard "Shred" Körber + * http://acme4j.shredzone.org + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + */ +package org.shredzone.acme4j.connector; + +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; + +import java.net.InetSocketAddress; +import java.net.Proxy; +import java.time.Duration; + +import org.junit.Test; + +/** + * Unit tests for {@link NetworkSettings}. + */ +public class NetworkSettingsTest { + + /** + * Test getters and setters. + */ + @Test + public void testGettersAndSetters() { + NetworkSettings settings = new NetworkSettings(); + + assertThat(settings.getProxy(), is(Proxy.NO_PROXY)); + Proxy proxy = new Proxy(Proxy.Type.HTTP, new InetSocketAddress("10.0.0.1", 8080)); + settings.setProxy(proxy); + assertThat(settings.getProxy(), is(proxy)); + settings.setProxy(null); + assertThat(settings.getProxy(), is(Proxy.NO_PROXY)); + + assertThat(settings.getTimeout(), is(Duration.ofSeconds(10))); + settings.setTimeout(Duration.ofMillis(5120)); + assertThat(settings.getTimeout(), is(Duration.ofMillis(5120))); + } + + @Test + public void testInvalidTimeouts() { + NetworkSettings settings = new NetworkSettings(); + + try { + settings.setTimeout(null); + fail("timeout accepted null"); + } catch (IllegalArgumentException ex) { + // expected + } + + try { + settings.setTimeout(Duration.ZERO); + fail("timeout accepted zero duration"); + } catch (IllegalArgumentException ex) { + // expected + } + + try { + settings.setTimeout(Duration.ofSeconds(20).negated()); + fail("timeout accepted negative duration"); + } catch (IllegalArgumentException ex) { + // expected + } + + try { + settings.setTimeout(Duration.ofMillis(Integer.MAX_VALUE + 1L)); + fail("timeout accepted out of range value"); + } catch (IllegalArgumentException ex) { + // expected + } + } + +} diff --git a/src/site/markdown/usage/session.md b/src/site/markdown/usage/session.md index e141552f..3a60e4cd 100644 --- a/src/site/markdown/usage/session.md +++ b/src/site/markdown/usage/session.md @@ -35,10 +35,11 @@ URL website = meta.getWebsite(); By default, the system's default locale is used. -## Proxy +## Network Settings -_acme4j_ uses a standard `HttpURLConnection` for HTTP connections. +_acme4j_ uses a standard `HttpURLConnection` for HTTP connections. You can use `Session.networkSettings()` to change some network parameters for the session. -If a proxy must be used for internet connections, you can set a `Proxy` instance by invoking `Session.setProxy()`. An alternative is to use the system properties `https.proxyHost` and `https.proxyPort` to globally set a proxy for the Java process. +* If a proxy must be used for internet connections, you can set a `Proxy` instance via `setProxy()`. An alternative is to use the system properties `https.proxyHost` and `https.proxyPort` to globally set a proxy for the Java process. +* To change network timeouts, use `setTimeout()`. The default timeout is 10 seconds. You can either increase the timeout on poor network connections, or reduce it to fail early on network errors. -If the proxy needs authentication, you need to set a default `Authenticator`. Be careful: Most code snippets I have found in the internet will send out the proxy credentials to anyone who is asking. See [this blog article](http://rolandtapken.de/blog/2012-04/java-process-httpproxyuser-and-httpproxypassword) for a good way to implement a proxy `Authenticator`. +If the proxy needs authentication, you need to set a default `Authenticator`. Be careful: Most code snippets I have found on the internet will send out the proxy credentials to anyone who is asking. See [this blog article](https://rolandtapken.de/blog/2012-04/java-process-httpproxyuser-and-httpproxypassword) for a good way to implement a proxy `Authenticator`.