Set User-Agent as header without middleware

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>

diff --git a/lib/private/Http/Client/Client.php b/lib/private/Http/Client/Client.php
index 03b09bf54b789e0790115d4381175b8962308d31..baa358f2fea9f2199814678498ae73242f4a6677 100644 (file)
--- a/lib/private/Http/Client/Client.php
+++ b/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\Middleware;
+use GuzzleHttp\RequestOptions;
 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,66 +52,57 @@ class Client implements IClient {
        public function __construct(
                IConfig $config,
                ICertificateManager $certificateManager,
-               GuzzleClient $client,
-               HandlerStack $stack
+               GuzzleClient $client
        ) {
                $this->config = $config;
                $this->client = $client;
-               $this->stack = $stack;
                $this->certificateManager = $certificateManager;
        }
 
-       /**
-        * Sets the default options to the client
-        */
-       private function setDefaultOptions() {
-               if ($this->configured) {
-                       return;
-               }
-               $this->configured = true;
+       private function buildRequestOptions(array $options): array {
+               $defaults = [
+                       RequestOptions::PROXY => $this->getProxyUri(),
+                       RequestOptions::VERIFY => $this->getCertBundle(),
+               ];
 
-               $this->stack->push(Middleware::mapRequest(function (RequestInterface $request) {
-                       return $request
-                               ->withHeader('User-Agent', 'Nextcloud Server Crawler');
-               }));
-       }
+               $options = array_merge($defaults, $options);
 
-       private function getRequestOptions() {
-               $options = [
-                       'verify' => $this->getCertBundle(),
-               ];
-               $proxyUri = $this->getProxyUri();
-               if ($proxyUri !== '') {
-                       $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);
                $proxyUserPwd = $this->config->getSystemValue('proxyuserpwd', null);
-               $proxyUri = '';
 
+               if ($proxyHost === null && $proxyUserPwd === null) {
+                       return null;
+               }
+
+               $proxyUri = '';
                if ($proxyUserPwd !== null) {
                        $proxyUri .= $proxyUserPwd . '@';
                }
@@ -157,8 +143,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, array_merge($this->getRequestOptions(), $options));
+               $response = $this->client->request('get', $uri, $this->buildRequestOptions($options));
                $isStream = isset($options['stream']) && $options['stream'];
                return new Response($response, $isStream);
        }
@@ -188,8 +173,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, array_merge($this->getRequestOptions(), $options));
+               $response = $this->client->request('head', $uri, $this->buildRequestOptions($options));
                return new Response($response);
        }
 
@@ -223,12 +207,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 +245,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, array_merge($this->getRequestOptions(), $options));
+               $response = $this->client->request('put', $uri, $this->buildRequestOptions($options));
                return new Response($response);
        }
 
@@ -297,12 +279,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, array_merge($this->getRequestOptions(), $options));
+               $response = $this->client->request('delete', $uri, $this->buildRequestOptions($options));
                return new Response($response);
        }
 
-
        /**
         * Sends a options request
         *
@@ -333,8 +313,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, array_merge($this->getRequestOptions(), $options));
+               $response = $this->client->request('options', $uri, $this->buildRequestOptions($options));
                return new Response($response);
        }
 }

diff --git a/lib/private/Http/Client/ClientService.php b/lib/private/Http/Client/ClientService.php
index fa8544f07a5c1fcff75c56d2d0739485aa537c80..1df54010a2d32a6d6243dc58e29e8e9b4f68f95c 100644 (file)
--- a/lib/private/Http/Client/ClientService.php
+++ b/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());
        }
 }

diff --git a/tests/lib/Http/Client/ClientServiceTest.php b/tests/lib/Http/Client/ClientServiceTest.php
index 1bfaf0503555d62b8618235b222021256ee29e98..02f331483dea60c6bc1bb111b22605c6a543a32e 100644 (file)
--- a/tests/lib/Http/Client/ClientServiceTest.php
+++ b/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()
+               );
        }
 }

diff --git a/tests/lib/Http/Client/ClientTest.php b/tests/lib/Http/Client/ClientTest.php
index 7f12a824d17f441df0faeb8974c58634fb79bac0..2fd23346398e67e13117b750002edea9f762fec4 100644 (file)
--- a/tests/lib/Http/Client/ClientTest.php
+++ b/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,19 +32,18 @@ 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,
-                       HandlerStack::create()
+                       $this->guzzleClient
                );
        }
 
-       public function testGetProxyUri() {
+       public function testGetProxyUri(): void {
                $this->config
                        ->expects($this->at(0))
                        ->method('getSystemValue')
@@ -56,10 +54,10 @@ class ClientTest extends \Test\TestCase {
                        ->method('getSystemValue')
                        ->with('proxyuserpwd', null)
                        ->willReturn(null);
-               $this->assertSame('', self::invokePrivate($this->client, 'getProxyUri'));
+               $this->assertNull(self::invokePrivate($this->client, 'getProxyUri'));
        }
 
-       public function testGetProxyUriProxyHostEmptyPassword() {
+       public function testGetProxyUriProxyHostEmptyPassword(): void {
                $this->config
                        ->expects($this->at(0))
                        ->method('getSystemValue')
@@ -73,7 +71,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 +85,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 +104,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 +120,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 +137,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 +146,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 +163,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 +172,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 +189,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 +198,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 +215,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 +224,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 +241,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 +250,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 +267,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(2))
                        ->method('getSystemValue')
                        ->with('installed', false)
                        ->willReturn(false);
@@ -260,11 +279,15 @@ class ClientTest extends \Test\TestCase {
                        ->willReturn([]);
 
                $this->assertEquals([
-                       'verify' => \OC::$SERVERROOT . '/resources/config/ca-bundle.crt'
-               ], self::invokePrivate($this->client, 'getRequestOptions'));
+                       'verify' => \OC::$SERVERROOT . '/resources/config/ca-bundle.crt',
+                       '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 +306,10 @@ class ClientTest extends \Test\TestCase {
 
                $this->assertEquals([
                        'verify' => '/my/path.crt',
-                       'proxy' => 'foo'
-               ], self::invokePrivate($this->client, 'getRequestOptions'));
+                       'proxy' => 'foo',
+                       'headers' => [
+                               'User-Agent' => 'Nextcloud Server Crawler'
+                       ]
+               ], self::invokePrivate($this->client, 'buildRequestOptions', [[]]));
        }
 }