Merge pull request #14363 from nextcloud/debt/cleanup-httpservice

Set User-Agent as header without middleware

lib/private/Http/Client/Client.php

@@ -25,13 +25,11 @@ declare(strict_types=1);
 namespace OC\Http\Client;
 
 use GuzzleHttp\Client as GuzzleClient;
-use GuzzleHttp\HandlerStack;
+use GuzzleHttp\RequestOptions;
-use GuzzleHttp\Middleware;
 use OCP\Http\Client\IClient;
 use OCP\Http\Client\IResponse;
 use OCP\ICertificateManager;
 use OCP\IConfig;
-use Psr\Http\Message\RequestInterface;
 
 /**
  * Class Client
@@ -45,9 +43,6 @@ class Client implements IClient {
 	private $config;
 	/** @var ICertificateManager */
 	private $certificateManager;
-	private $configured = false;
-	/** @var HandlerStack */
-	private $stack;
 
 	/**
 	 * @param IConfig $config
@@ -57,74 +52,62 @@ class Client implements IClient {
 	public function __construct(
 		IConfig $config,
 		ICertificateManager $certificateManager,
-		GuzzleClient $client,
+		GuzzleClient $client
-		HandlerStack $stack
 	) {
 		$this->config = $config;
 		$this->client = $client;
-		$this->stack = $stack;
 		$this->certificateManager = $certificateManager;
 	}
 
-	/**
+	private function buildRequestOptions(array $options): array {
-	 * Sets the default options to the client
+		$defaults = [
-	 */
+			RequestOptions::PROXY => $this->getProxyUri(),
-	private function setDefaultOptions() {
+			RequestOptions::VERIFY => $this->getCertBundle(),
-		if ($this->configured) {
-			return;
-		}
-		$this->configured = true;
-
-		$this->stack->push(Middleware::mapRequest(function (RequestInterface $request) {
-			return $request
-				->withHeader('User-Agent', 'Nextcloud Server Crawler');
-		}));
-	}
-
-	private function getRequestOptions() {
-		$options = [
-			'verify' => $this->getCertBundle(),
 		];
-		$proxyUri = $this->getProxyUri();
+
-		if ($proxyUri !== '') {
+		$options = array_merge($defaults, $options);
-			$options['proxy'] = $proxyUri;
+
+		if (!isset($options[RequestOptions::HEADERS]['User-Agent'])) {
+			$options[RequestOptions::HEADERS]['User-Agent'] = 'Nextcloud Server Crawler';
 		}
+
 		return $options;
 	}
 
-	private function getCertBundle() {
+	private function getCertBundle(): string {
 		if ($this->certificateManager->listCertificates() !== []) {
 			return $this->certificateManager->getAbsoluteBundlePath();
-		} else {
-			// If the instance is not yet setup we need to use the static path as
-			// $this->certificateManager->getAbsoluteBundlePath() tries to instantiiate
-			// a view
-			if ($this->config->getSystemValue('installed', false)) {
-				return $this->certificateManager->getAbsoluteBundlePath(null);
-			} else {
-				return \OC::$SERVERROOT . '/resources/config/ca-bundle.crt';
-			}
 		}
+
+		// If the instance is not yet setup we need to use the static path as
+		// $this->certificateManager->getAbsoluteBundlePath() tries to instantiiate
+		// a view
+		if ($this->config->getSystemValue('installed', false)) {
+			return $this->certificateManager->getAbsoluteBundlePath(null);
+		}
+
+		return \OC::$SERVERROOT . '/resources/config/ca-bundle.crt';
 	}
 
 	/**
 	 * Get the proxy URI
 	 *
-	 * @return string
+	 * @return string|null
 	 */
-	private function getProxyUri(): string {
+	private function getProxyUri(): ?string {
 		$proxyHost = $this->config->getSystemValue('proxy', null);
+
+		if ($proxyHost === null) {
+			return null;
+		}
+
 		$proxyUserPwd = $this->config->getSystemValue('proxyuserpwd', null);
-		$proxyUri = '';
 
-		if ($proxyUserPwd !== null) {
+		if ($proxyUserPwd === null) {
-			$proxyUri .= $proxyUserPwd . '@';
+			return $proxyHost;
-		}
-		if ($proxyHost !== null) {
-			$proxyUri .= $proxyHost;
 		}
 
-		return $proxyUri;
+		return $proxyUserPwd . '@' . $proxyHost;
 	}
 
 	/**
@@ -157,8 +140,7 @@ class Client implements IClient {
 	 * @throws \Exception If the request could not get completed
 	 */
 	public function get(string $uri, array $options = []): IResponse {
-		$this->setDefaultOptions();
+		$response = $this->client->request('get', $uri, $this->buildRequestOptions($options));
-		$response = $this->client->request('get', $uri, array_merge($this->getRequestOptions(), $options));
 		$isStream = isset($options['stream']) && $options['stream'];
 		return new Response($response, $isStream);
 	}
@@ -188,8 +170,7 @@ class Client implements IClient {
 	 * @throws \Exception If the request could not get completed
 	 */
 	public function head(string $uri, array $options = []): IResponse {
-		$this->setDefaultOptions();
+		$response = $this->client->request('head', $uri, $this->buildRequestOptions($options));
-		$response = $this->client->request('head', $uri, array_merge($this->getRequestOptions(), $options));
 		return new Response($response);
 	}
 
@@ -223,12 +204,11 @@ class Client implements IClient {
 	 * @throws \Exception If the request could not get completed
 	 */
 	public function post(string $uri, array $options = []): IResponse {
-		$this->setDefaultOptions();
 		if (isset($options['body']) && is_array($options['body'])) {
 			$options['form_params'] = $options['body'];
 			unset($options['body']);
 		}
-		$response = $this->client->request('post', $uri, array_merge($this->getRequestOptions(), $options));
+		$response = $this->client->request('post', $uri, $this->buildRequestOptions($options));
 		return new Response($response);
 	}
 
@@ -262,8 +242,7 @@ class Client implements IClient {
 	 * @throws \Exception If the request could not get completed
 	 */
 	public function put(string $uri, array $options = []): IResponse {
-		$this->setDefaultOptions();
+		$response = $this->client->request('put', $uri, $this->buildRequestOptions($options));
-		$response = $this->client->request('put', $uri, array_merge($this->getRequestOptions(), $options));
 		return new Response($response);
 	}
 
@@ -297,12 +276,10 @@ class Client implements IClient {
 	 * @throws \Exception If the request could not get completed
 	 */
 	public function delete(string $uri, array $options = []): IResponse {
-		$this->setDefaultOptions();
+		$response = $this->client->request('delete', $uri, $this->buildRequestOptions($options));
-		$response = $this->client->request('delete', $uri, array_merge($this->getRequestOptions(), $options));
 		return new Response($response);
 	}
 
-
 	/**
 	 * Sends a options request
 	 *
@@ -333,8 +310,7 @@ class Client implements IClient {
 	 * @throws \Exception If the request could not get completed
 	 */
 	public function options(string $uri, array $options = []): IResponse {
-		$this->setDefaultOptions();
+		$response = $this->client->request('options', $uri, $this->buildRequestOptions($options));
-		$response = $this->client->request('options', $uri, array_merge($this->getRequestOptions(), $options));
 		return new Response($response);
 	}
 }

lib/private/Http/Client/ClientService.php

@@ -24,7 +24,6 @@ declare(strict_types=1);
 namespace OC\Http\Client;
 
 use GuzzleHttp\Client as GuzzleClient;
-use GuzzleHttp\HandlerStack;
 use OCP\Http\Client\IClient;
 use OCP\Http\Client\IClientService;
 use OCP\ICertificateManager;
@@ -55,6 +54,6 @@ class ClientService implements IClientService {
 	 * @return Client
 	 */
 	public function newClient(): IClient {
-		return new Client($this->config, $this->certificateManager, new GuzzleClient(), HandlerStack::create());
+		return new Client($this->config, $this->certificateManager, new GuzzleClient());
 	}
 }

tests/lib/Http/Client/ClientServiceTest.php

@@ -9,7 +9,6 @@
 namespace Test\Http\Client;
 
 use GuzzleHttp\Client as GuzzleClient;
-use GuzzleHttp\HandlerStack;
 use OC\Http\Client\Client;
 use OC\Http\Client\ClientService;
 use OCP\ICertificateManager;
@@ -19,12 +18,16 @@ use OCP\IConfig;
  * Class ClientServiceTest
  */
 class ClientServiceTest extends \Test\TestCase {
-	public function testNewClient() {
+	public function testNewClient(): void {
+		/** @var IConfig $config */
 		$config = $this->createMock(IConfig::class);
+		/** @var ICertificateManager $certificateManager */
 		$certificateManager = $this->createMock(ICertificateManager::class);
 
-		$expected = new Client($config, $certificateManager, new GuzzleClient(), HandlerStack::create());
 		$clientService = new ClientService($config, $certificateManager);
-		$this->assertEquals($expected, $clientService->newClient());
+		$this->assertEquals(
+			new Client($config, $certificateManager, new GuzzleClient()),
+			$clientService->newClient()
+		);
 	}
 }

tests/lib/Http/Client/ClientTest.php

@@ -8,7 +8,6 @@
 
 namespace Test\Http\Client;
 
-use GuzzleHttp\HandlerStack;
 use GuzzleHttp\Psr7\Response;
 use OC\Http\Client\Client;
 use OC\Security\CertificateManager;
@@ -33,33 +32,27 @@ class ClientTest extends \Test\TestCase {
 	public function setUp() {
 		parent::setUp();
 		$this->config = $this->createMock(IConfig::class);
-		$this->guzzleClient = $this->getMockBuilder('\GuzzleHttp\Client')
+		$this->guzzleClient = $this->getMockBuilder(\GuzzleHttp\Client::class)
 			->disableOriginalConstructor()
 			->getMock();
 		$this->certificateManager = $this->createMock(ICertificateManager::class);
 		$this->client = new Client(
 			$this->config,
 			$this->certificateManager,
-			$this->guzzleClient,
+			$this->guzzleClient
-			HandlerStack::create()
 		);
 	}
 
-	public function testGetProxyUri() {
+	public function testGetProxyUri(): void {
 		$this->config
 			->expects($this->at(0))
 			->method('getSystemValue')
 			->with('proxy', null)
 			->willReturn(null);
-		$this->config
+		$this->assertNull(self::invokePrivate($this->client, 'getProxyUri'));
-			->expects($this->at(1))
-			->method('getSystemValue')
-			->with('proxyuserpwd', null)
-			->willReturn(null);
-		$this->assertSame('', self::invokePrivate($this->client, 'getProxyUri'));
 	}
 
-	public function testGetProxyUriProxyHostEmptyPassword() {
+	public function testGetProxyUriProxyHostEmptyPassword(): void {
 		$this->config
 			->expects($this->at(0))
 			->method('getSystemValue')
@@ -73,7 +66,7 @@ class ClientTest extends \Test\TestCase {
 		$this->assertSame('foo', self::invokePrivate($this->client, 'getProxyUri'));
 	}
 
-	public function testGetProxyUriProxyHostWithPassword() {
+	public function testGetProxyUriProxyHostWithPassword(): void {
 		$this->config
 			->expects($this->at(0))
 			->method('getSystemValue')
@@ -87,7 +80,7 @@ class ClientTest extends \Test\TestCase {
 		$this->assertSame('username:password@foo', self::invokePrivate($this->client, 'getProxyUri'));
 	}
 
-	private function setUpDefaultRequestOptions() {
+	private function setUpDefaultRequestOptions(): void {
 		$this->config
 			->expects($this->at(0))
 			->method('getSystemValue')
@@ -106,11 +99,14 @@ class ClientTest extends \Test\TestCase {
 
 		$this->defaultRequestOptions = [
 			'verify' => '/my/path.crt',
-			'proxy' => 'foo'
+			'proxy' => 'foo',
+			'headers' => [
+				'User-Agent' => 'Nextcloud Server Crawler'
+			]
 		];
 	}
 
-	public function testGet() {
+	public function testGet(): void {
 		$this->setUpDefaultRequestOptions();
 
 		$this->guzzleClient->method('request')
@@ -119,12 +115,15 @@ class ClientTest extends \Test\TestCase {
 		$this->assertEquals(1337, $this->client->get('http://localhost/', [])->getStatusCode());
 	}
 
-	public function testGetWithOptions() {
+	public function testGetWithOptions(): void {
 		$this->setUpDefaultRequestOptions();
 
 		$options = [
 			'verify' => false,
-			'proxy' => 'bar'
+			'proxy' => 'bar',
+			'headers' => [
+				'User-Agent' => 'Nextcloud Server Crawler'
+			]
 		];
 
 		$this->guzzleClient->method('request')
@@ -133,7 +132,7 @@ class ClientTest extends \Test\TestCase {
 		$this->assertEquals(1337, $this->client->get('http://localhost/', $options)->getStatusCode());
 	}
 
-	public function testPost() {
+	public function testPost(): void {
 		$this->setUpDefaultRequestOptions();
 
 		$this->guzzleClient->method('request')
@@ -142,12 +141,15 @@ class ClientTest extends \Test\TestCase {
 		$this->assertEquals(1337, $this->client->post('http://localhost/', [])->getStatusCode());
 	}
 
-	public function testPostWithOptions() {
+	public function testPostWithOptions(): void {
 		$this->setUpDefaultRequestOptions();
 
 		$options = [
 			'verify' => false,
-			'proxy' => 'bar'
+			'proxy' => 'bar',
+			'headers' => [
+				'User-Agent' => 'Nextcloud Server Crawler'
+			]
 		];
 
 		$this->guzzleClient->method('request')
@@ -156,7 +158,7 @@ class ClientTest extends \Test\TestCase {
 		$this->assertEquals(1337, $this->client->post('http://localhost/', $options)->getStatusCode());
 	}
 
-	public function testPut() {
+	public function testPut(): void {
 		$this->setUpDefaultRequestOptions();
 
 		$this->guzzleClient->method('request')
@@ -165,12 +167,15 @@ class ClientTest extends \Test\TestCase {
 		$this->assertEquals(1337, $this->client->put('http://localhost/', [])->getStatusCode());
 	}
 
-	public function testPutWithOptions() {
+	public function testPutWithOptions(): void {
 		$this->setUpDefaultRequestOptions();
 
 		$options = [
 			'verify' => false,
-			'proxy' => 'bar'
+			'proxy' => 'bar',
+			'headers' => [
+				'User-Agent' => 'Nextcloud Server Crawler'
+			]
 		];
 
 		$this->guzzleClient->method('request')
@@ -179,7 +184,7 @@ class ClientTest extends \Test\TestCase {
 		$this->assertEquals(1337, $this->client->put('http://localhost/', $options)->getStatusCode());
 	}
 
-	public function testDelete() {
+	public function testDelete(): void {
 		$this->setUpDefaultRequestOptions();
 
 		$this->guzzleClient->method('request')
@@ -188,12 +193,15 @@ class ClientTest extends \Test\TestCase {
 		$this->assertEquals(1337, $this->client->delete('http://localhost/', [])->getStatusCode());
 	}
 
-	public function testDeleteWithOptions() {
+	public function testDeleteWithOptions(): void {
 		$this->setUpDefaultRequestOptions();
 
 		$options = [
 			'verify' => false,
-			'proxy' => 'bar'
+			'proxy' => 'bar',
+			'headers' => [
+				'User-Agent' => 'Nextcloud Server Crawler'
+			]
 		];
 
 		$this->guzzleClient->method('request')
@@ -202,7 +210,7 @@ class ClientTest extends \Test\TestCase {
 		$this->assertEquals(1337, $this->client->delete('http://localhost/', $options)->getStatusCode());
 	}
 
-	public function testOptions() {
+	public function testOptions(): void {
 		$this->setUpDefaultRequestOptions();
 
 		$this->guzzleClient->method('request')
@@ -211,12 +219,15 @@ class ClientTest extends \Test\TestCase {
 		$this->assertEquals(1337, $this->client->options('http://localhost/', [])->getStatusCode());
 	}
 
-	public function testOptionsWithOptions() {
+	public function testOptionsWithOptions(): void {
 		$this->setUpDefaultRequestOptions();
 
 		$options = [
 			'verify' => false,
-			'proxy' => 'bar'
+			'proxy' => 'bar',
+			'headers' => [
+				'User-Agent' => 'Nextcloud Server Crawler'
+			]
 		];
 
 		$this->guzzleClient->method('request')
@@ -225,7 +236,7 @@ class ClientTest extends \Test\TestCase {
 		$this->assertEquals(1337, $this->client->options('http://localhost/', $options)->getStatusCode());
 	}
 
-	public function testHead() {
+	public function testHead(): void {
 		$this->setUpDefaultRequestOptions();
 
 		$this->guzzleClient->method('request')
@@ -234,12 +245,15 @@ class ClientTest extends \Test\TestCase {
 		$this->assertEquals(1337, $this->client->head('http://localhost/', [])->getStatusCode());
 	}
 
-	public function testHeadWithOptions() {
+	public function testHeadWithOptions(): void {
 		$this->setUpDefaultRequestOptions();
 
 		$options = [
 			'verify' => false,
-			'proxy' => 'bar'
+			'proxy' => 'bar',
+			'headers' => [
+				'User-Agent' => 'Nextcloud Server Crawler'
+			]
 		];
 
 		$this->guzzleClient->method('request')
@@ -248,9 +262,9 @@ class ClientTest extends \Test\TestCase {
 		$this->assertEquals(1337, $this->client->head('http://localhost/', $options)->getStatusCode());
 	}
 
-	public function testSetDefaultOptionsWithNotInstalled() {
+	public function testSetDefaultOptionsWithNotInstalled(): void {
 		$this->config
-			->expects($this->at(0))
+			->expects($this->at(1))
 			->method('getSystemValue')
 			->with('installed', false)
 			->willReturn(false);
@@ -260,11 +274,15 @@ class ClientTest extends \Test\TestCase {
 			->willReturn([]);
 
 		$this->assertEquals([
-			'verify' => \OC::$SERVERROOT . '/resources/config/ca-bundle.crt'
+			'verify' => \OC::$SERVERROOT . '/resources/config/ca-bundle.crt',
-		], self::invokePrivate($this->client, 'getRequestOptions'));
+			'proxy' => null,
+			'headers' => [
+				'User-Agent' => 'Nextcloud Server Crawler'
+			]
+		], self::invokePrivate($this->client, 'buildRequestOptions', [[]]));
 	}
 
-	public function testSetDefaultOptionsWithProxy() {
+	public function testSetDefaultOptionsWithProxy(): void {
 		$this->config
 			->expects($this->at(0))
 			->method('getSystemValue')
@@ -283,7 +301,10 @@ class ClientTest extends \Test\TestCase {
 
 		$this->assertEquals([
 			'verify' => '/my/path.crt',
-			'proxy' => 'foo'
+			'proxy' => 'foo',
-		], self::invokePrivate($this->client, 'getRequestOptions'));
+			'headers' => [
+				'User-Agent' => 'Nextcloud Server Crawler'
+			]
+		], self::invokePrivate($this->client, 'buildRequestOptions', [[]]));
 	}
 }