From 44f25d6b15188fbc430146d5e21f4baf9f866b6d Mon Sep 17 00:00:00 2001 From: Konstantin Schaper Date: Wed, 25 Aug 2021 08:19:52 +0200 Subject: [PATCH] Fix disabled local proxy configuration being used over global config (#1780) The original proxy configuration implementation only used the global configuration if the local proxy configuration was not provided (i.e. null). This PR adds the corner case where a local configuration is provided, but disabled. In this case, the global proxy configuration will be used as a fallback as well. --- .../disabled_local_proxy_config.yaml | 2 ++ .../scm/net/HttpURLConnectionFactory.java | 2 +- .../scm/net/HttpURLConnectionFactoryTest.java | 17 ++++++++++++++- .../scm/repository/spi/SvnMirrorCommand.java | 2 +- .../repository/spi/SvnMirrorCommandTest.java | 21 +++++++++++++++++++ 5 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 gradle/changelog/disabled_local_proxy_config.yaml diff --git a/gradle/changelog/disabled_local_proxy_config.yaml b/gradle/changelog/disabled_local_proxy_config.yaml new file mode 100644 index 0000000000..469a24ee02 --- /dev/null +++ b/gradle/changelog/disabled_local_proxy_config.yaml @@ -0,0 +1,2 @@ +- type: Fixed + description: Fix disabled local proxy configuration being used over global config ([#1780](https://github.com/scm-manager/scm-manager/pull/1780)) diff --git a/scm-core/src/main/java/sonia/scm/net/HttpURLConnectionFactory.java b/scm-core/src/main/java/sonia/scm/net/HttpURLConnectionFactory.java index 691dc2845c..49704cf6f0 100644 --- a/scm-core/src/main/java/sonia/scm/net/HttpURLConnectionFactory.java +++ b/scm-core/src/main/java/sonia/scm/net/HttpURLConnectionFactory.java @@ -129,7 +129,7 @@ public final class HttpURLConnectionFactory { // because we are not able to remove the authentication from thread local ThreadLocalAuthenticator.clear(); - ProxyConfiguration proxyConfiguration = options.getProxyConfiguration().orElse(globalProxyConfiguration); + ProxyConfiguration proxyConfiguration = options.getProxyConfiguration().filter(ProxyConfiguration::isEnabled).orElse(globalProxyConfiguration); if (isProxyEnabled(proxyConfiguration, url)) { return openProxyConnection(proxyConfiguration, url); } diff --git a/scm-core/src/test/java/sonia/scm/net/HttpURLConnectionFactoryTest.java b/scm-core/src/test/java/sonia/scm/net/HttpURLConnectionFactoryTest.java index fdcbe454b9..9652dbac1f 100644 --- a/scm-core/src/test/java/sonia/scm/net/HttpURLConnectionFactoryTest.java +++ b/scm-core/src/test/java/sonia/scm/net/HttpURLConnectionFactoryTest.java @@ -180,6 +180,22 @@ class HttpURLConnectionFactoryTest { verify(connection).setReadTimeout(3000); } + @Test + void shouldUseGlobalProxyConfigurationIfLocalOneIsDisabled() throws IOException { + configuration.setProxyServer("proxy.hitchhiker.com"); + configuration.setProxyPort(3128); + configuration.setEnableProxy(true); + + ScmConfiguration localProxyConf = new ScmConfiguration(); + localProxyConf.setEnableProxy(false); + localProxyConf.setProxyServer("prox.hitchhiker.net"); + localProxyConf.setProxyPort(3127); + + connectionFactory.create(new URL("https://hitchhiker.org"), new HttpConnectionOptions().withProxyConfiguration(new GlobalProxyConfiguration(localProxyConf))); + + assertUsedProxy("proxy.hitchhiker.com", 3128); + } + @Test void shouldCreateProxyConnection() throws IOException { configuration.setEnableProxy(true); @@ -341,7 +357,6 @@ class HttpURLConnectionFactoryTest { return captor.getValue(); } - private void assertUsedProxy(String host, int port) { assertThat(usedProxy).isNotNull(); assertThat(usedProxy.address()).isInstanceOfSatisfying(InetSocketAddress.class, inet -> { diff --git a/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SvnMirrorCommand.java b/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SvnMirrorCommand.java index a759266ee1..5ac6274ab0 100644 --- a/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SvnMirrorCommand.java +++ b/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SvnMirrorCommand.java @@ -147,7 +147,7 @@ public class SvnMirrorCommand extends AbstractSvnCommand implements MirrorComman } }; checkAndApplyProxyConfiguration( - authManager, mirrorCommandRequest.getProxyConfiguration().orElse(globalProxyConfiguration), url + authManager, mirrorCommandRequest.getProxyConfiguration().filter(ProxyConfiguration::isEnabled).orElse(globalProxyConfiguration), url ); return authManager; } diff --git a/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnMirrorCommandTest.java b/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnMirrorCommandTest.java index 962a992ae1..811b977832 100644 --- a/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnMirrorCommandTest.java +++ b/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnMirrorCommandTest.java @@ -53,6 +53,7 @@ import java.util.function.Consumer; import static java.util.Collections.singleton; import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static sonia.scm.repository.api.MirrorCommandResult.ResultType.OK; @@ -197,6 +198,18 @@ public class SvnMirrorCommandTest extends AbstractSvnCommandTestBase { assertThat(authenticationManager.getProxyPasswordValue()).isNull(); } + @Test + public void shouldNotApplyDisabledLocalProxySettings() throws SVNException { + MirrorCommandRequest request = new MirrorCommandRequest(); + request.setProxyConfiguration(createDisabledProxyConfiguration()); + + BasicAuthenticationManager authenticationManager = createAuthenticationManager(request); + assertThat(authenticationManager.getProxyHost()).isNull(); + assertThat(authenticationManager.getProxyPort()).isZero(); + assertThat(authenticationManager.getProxyUserName()).isNull(); + assertThat(authenticationManager.getProxyPasswordValue()).isNull(); + } + private ProxyConfiguration createProxyConfiguration() { ProxyConfiguration configuration = mock(ProxyConfiguration.class); when(configuration.isEnabled()).thenReturn(true); @@ -205,6 +218,14 @@ public class SvnMirrorCommandTest extends AbstractSvnCommandTestBase { return configuration; } + private ProxyConfiguration createDisabledProxyConfiguration() { + ProxyConfiguration configuration = mock(ProxyConfiguration.class); + when(configuration.isEnabled()).thenReturn(false); + lenient().when(configuration.getHost()).thenReturn("proxy.hitchhiker.com"); + lenient().when(configuration.getPort()).thenReturn(3128); + return configuration; + } + private BasicAuthenticationManager createAuthenticationManager() throws SVNException { return createAuthenticationManager(new MirrorCommandRequest()); }