Merge pull request #17684 from mlatief/support-no-proxy

Add support for GuzzleHTTP 'no' proxy
2020-03-22 19:38:01 +01:00 · 2020-03-22 19:38:01 +01:00 · 6675f9b403
parent ea6601a2fc 98d6415264
commit 6675f9b403
3 changed files with 190 additions and 19 deletions
--- a/config/config.sample.php
+++ b/config/config.sample.php
@ -524,6 +524,12 @@ $CONFIG = array(
 /**
 * The URL of your proxy server, for example ``proxy.example.com:8081``.
 *
+ * Note: Guzzle (the http library used by Nextcloud) is reading the environment
+ * variables HTTP_PROXY (only for cli request), HTTPS_PROXY, and NO_PROXY by default.
+ *
+ * If you configure proxy with Nextcloud any default configuration by Guzzle
+ * is overwritten. Make sure to set ``proxyexclude`` accordingly if necessary.
+ *
 * Defaults to ``''`` (empty string)
 */
 'proxy' => '',
@ -536,6 +542,16 @@ $CONFIG = array(
 */
 'proxyuserpwd' => '',

+/**
+ * List of host names that should not be proxied to.
+ * For example: ``['.mit.edu', 'foo.com']``.
+ *
+ * Hint: Use something like ``explode(',', getenv('NO_PROXY'))`` to sync this
+ * value with the global NO_PROXY option.
+ *
+ * Defaults to empty array.
+ */
+'proxyexclude' => [],

 /**
 * Deleted Items (trash bin)
--- a/lib/private/Http/Client/Client.php
+++ b/lib/private/Http/Client/Client.php
@ -65,12 +65,20 @@ class Client implements IClient {
 	}

 	private function buildRequestOptions(array $options): array {
+		$proxy = $this->getProxyUri();
+
 		$defaults = [
-			RequestOptions::PROXY => $this->getProxyUri(),
 			RequestOptions::VERIFY => $this->getCertBundle(),
 			RequestOptions::TIMEOUT => 30,
 		];

+		// Only add RequestOptions::PROXY if Nextcloud is explicitly
+		// configured to use a proxy. This is needed in order not to override
+		// Guzzle default values.
+		if($proxy !== null) {
+			$defaults[RequestOptions::PROXY] = $proxy;
+		}
+
 		$options = array_merge($defaults, $options);

 		if (!isset($options[RequestOptions::HEADERS]['User-Agent'])) {
@ -96,11 +104,21 @@ class Client implements IClient {
 	}

 	/**
-	 * Get the proxy URI
+	 * Returns a null or an associative array specifiying the proxy URI for
+	 * 'http' and 'https' schemes, in addition to a 'no' key value pair
+	 * providing a list of host names that should not be proxied to.
+	 *
+	 * @return array|null
+	 *
+	 * The return array looks like:
+	 * [
+	 *   'http' => 'username:password@proxy.example.com',
+	 *   'https' => 'username:password@proxy.example.com',
+	 *   'no' => ['foo.com', 'bar.com']
+	 * ]
 	 *
-	 * @return string|null
 	 */
-	private function getProxyUri(): ?string {
+	private function getProxyUri(): ?array {
 		$proxyHost = $this->config->getSystemValue('proxy', '');

 		if ($proxyHost === '' || $proxyHost === null) {
@ -108,12 +126,21 @@ class Client implements IClient {
 		}

 		$proxyUserPwd = $this->config->getSystemValue('proxyuserpwd', '');
-
-		if ($proxyUserPwd === '' || $proxyUserPwd === null) {
-			return $proxyHost;
+		if ($proxyUserPwd !== '' && $proxyUserPwd !== null) {
+			$proxyHost = $proxyUserPwd . '@' . $proxyHost;
 		}

-		return $proxyUserPwd . '@' . $proxyHost;
+		$proxy = [
+			'http' => $proxyHost,
+			'https' => $proxyHost,
+		];
+
+		$proxyExclude = $this->config->getSystemValue('proxyexclude', []);
+		if ($proxyExclude !== [] && $proxyExclude !== null) {
+			$proxy['no'] = $proxyExclude;
+		}
+
+		return $proxy;
 	}

 	/**
--- a/tests/lib/Http/Client/ClientTest.php
+++ b/tests/lib/Http/Client/ClientTest.php
@ -63,7 +63,15 @@ class ClientTest extends \Test\TestCase {
 			->method('getSystemValue')
 			->with('proxyuserpwd', null)
 			->willReturn(null);
-		$this->assertSame('foo', self::invokePrivate($this->client, 'getProxyUri'));
+		$this->config
+			->expects($this->at(2))
+			->method('getSystemValue')
+			->with('proxyexclude', [])
+			->willReturn([]);
+		$this->assertEquals([
+			'http' => 'foo',
+			'https' => 'foo'
+		], self::invokePrivate($this->client, 'getProxyUri'));
 	}

 	public function testGetProxyUriProxyHostWithPassword(): void {
@ -87,7 +95,58 @@ class ClientTest extends \Test\TestCase {
 				})
 			)
 			->willReturn('username:password');
-		$this->assertSame('username:password@foo', self::invokePrivate($this->client, 'getProxyUri'));
+		$this->config
+			->expects($this->at(2))
+			->method('getSystemValue')
+			->with(
+				$this->equalTo('proxyexclude'),
+				$this->callback(function ($input) {
+					return $input === [];
+				})
+			)
+			->willReturn([]);
+		$this->assertEquals([
+			'http' => 'username:password@foo',
+			'https' => 'username:password@foo'
+		], self::invokePrivate($this->client, 'getProxyUri'));
+	}
+
+	public function testGetProxyUriProxyHostWithPasswordAndExclude(): void {
+		$this->config
+			->expects($this->at(0))
+			->method('getSystemValue')
+			->with(
+				$this->equalTo('proxy'),
+				$this->callback(function ($input) {
+					return $input === '';
+				})
+			)
+			->willReturn('foo');
+		$this->config
+			->expects($this->at(1))
+			->method('getSystemValue')
+			->with(
+				$this->equalTo('proxyuserpwd'),
+				$this->callback(function ($input) {
+					return $input === '';
+				})
+			)
+			->willReturn('username:password');
+		$this->config
+			->expects($this->at(2))
+			->method('getSystemValue')
+			->with(
+				$this->equalTo('proxyexclude'),
+				$this->callback(function ($input) {
+					return $input === [];
+				})
+			)
+			->willReturn(['bar']);
+		$this->assertEquals([
+			'http' => 'username:password@foo',
+			'https' => 'username:password@foo',
+			'no' => ['bar']
+		], self::invokePrivate($this->client, 'getProxyUri'));
 	}

 	private function setUpDefaultRequestOptions(): void {
@ -101,6 +160,11 @@ class ClientTest extends \Test\TestCase {
 			->method('getSystemValue')
 			->with('proxyuserpwd', null)
 			->willReturn(null);
+		$this->config
+			->expects($this->at(2))
+			->method('getSystemValue')
+			->with('proxyexclude', [])
+			->willReturn([]);
 		$this->certificateManager
 			->expects($this->once())
 			->method('getAbsoluteBundlePath')
@ -109,7 +173,10 @@ class ClientTest extends \Test\TestCase {

 		$this->defaultRequestOptions = [
 			'verify' => '/my/path.crt',
-			'proxy' => 'foo',
+			'proxy' => [
+				'http' => 'foo',
+				'https' => 'foo'
+			],
 			'headers' => [
 				'User-Agent' => 'Nextcloud Server Crawler',
 			],
@ -131,7 +198,10 @@ class ClientTest extends \Test\TestCase {

 		$options = array_merge($this->defaultRequestOptions, [
 			'verify' => false,
-			'proxy' => 'bar',
+			'proxy' => [
+				'http' => 'bar',
+				'https' => 'bar'
+			],
 		]);

 		$this->guzzleClient->method('request')
@ -154,7 +224,10 @@ class ClientTest extends \Test\TestCase {

 		$options = array_merge($this->defaultRequestOptions, [
 			'verify' => false,
-			'proxy' => 'bar',
+			'proxy' => [
+				'http' => 'bar',
+				'https' => 'bar'
+			],
 		]);

 		$this->guzzleClient->method('request')
@ -177,7 +250,10 @@ class ClientTest extends \Test\TestCase {

 		$options = array_merge($this->defaultRequestOptions, [
 			'verify' => false,
-			'proxy' => 'bar',
+			'proxy' => [
+				'http' => 'bar',
+				'https' => 'bar'
+			],
 		]);

 		$this->guzzleClient->method('request')
@ -200,7 +276,10 @@ class ClientTest extends \Test\TestCase {

 		$options = array_merge($this->defaultRequestOptions, [
 			'verify' => false,
-			'proxy' => 'bar',
+			'proxy' => [
+				'http' => 'bar',
+				'https' => 'bar'
+			],
 		]);

 		$this->guzzleClient->method('request')
@ -223,7 +302,10 @@ class ClientTest extends \Test\TestCase {

 		$options = array_merge($this->defaultRequestOptions, [
 			'verify' => false,
-			'proxy' => 'bar',
+			'proxy' => [
+				'http' => 'bar',
+				'https' => 'bar'
+			],
 		]);

 		$this->guzzleClient->method('request')
@ -246,7 +328,10 @@ class ClientTest extends \Test\TestCase {

 		$options = array_merge($this->defaultRequestOptions, [
 			'verify' => false,
-			'proxy' => 'bar',
+			'proxy' => [
+				'http' => 'bar',
+				'https' => 'bar'
+			],
 		]);

 		$this->guzzleClient->method('request')
@ -268,7 +353,6 @@ class ClientTest extends \Test\TestCase {

 		$this->assertEquals([
 			'verify' => \OC::$SERVERROOT . '/resources/config/ca-bundle.crt',
-			'proxy' => null,
 			'headers' => [
 				'User-Agent' => 'Nextcloud Server Crawler'
 			],
@ -287,6 +371,11 @@ class ClientTest extends \Test\TestCase {
 			->method('getSystemValue')
 			->with('proxyuserpwd', null)
 			->willReturn(null);
+		$this->config
+			->expects($this->at(2))
+			->method('getSystemValue')
+			->with('proxyexclude', [])
+			->willReturn([]);
 		$this->certificateManager
 			->expects($this->once())
 			->method('getAbsoluteBundlePath')
@ -295,7 +384,46 @@ class ClientTest extends \Test\TestCase {

 		$this->assertEquals([
 			'verify' => '/my/path.crt',
-			'proxy' => 'foo',
+			'proxy' => [
+				'http' => 'foo',
+				'https' => 'foo'
+			],
+			'headers' => [
+				'User-Agent' => 'Nextcloud Server Crawler'
+			],
+			'timeout' => 30,
+		], self::invokePrivate($this->client, 'buildRequestOptions', [[]]));
+	}
+
+	public function testSetDefaultOptionsWithProxyAndExclude(): void {
+		$this->config
+			->expects($this->at(0))
+			->method('getSystemValue')
+			->with('proxy', null)
+			->willReturn('foo');
+		$this->config
+			->expects($this->at(1))
+			->method('getSystemValue')
+			->with('proxyuserpwd', null)
+			->willReturn(null);
+		$this->config
+			->expects($this->at(2))
+			->method('getSystemValue')
+			->with('proxyexclude', [])
+			->willReturn(['bar']);
+		$this->certificateManager
+			->expects($this->once())
+			->method('getAbsoluteBundlePath')
+			->with(null)
+			->willReturn('/my/path.crt');
+
+		$this->assertEquals([
+			'verify' => '/my/path.crt',
+			'proxy' => [
+				'http' => 'foo',
+				'https' => 'foo',
+				'no' => ['bar']
+			],
 			'headers' => [
 				'User-Agent' => 'Nextcloud Server Crawler'
 			],