diff --git a/lib/private/Http/Client/Client.php b/lib/private/Http/Client/Client.php index 03b09bf54b..ad51feca0b 100644 --- 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,74 +52,62 @@ 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; - - $this->stack->push(Middleware::mapRequest(function (RequestInterface $request) { - return $request - ->withHeader('User-Agent', 'Nextcloud Server Crawler'); - })); - } - - private function getRequestOptions() { - $options = [ - 'verify' => $this->getCertBundle(), + private function buildRequestOptions(array $options): array { + $defaults = [ + RequestOptions::PROXY => $this->getProxyUri(), + RequestOptions::VERIFY => $this->getCertBundle(), ]; - $proxyUri = $this->getProxyUri(); - if ($proxyUri !== '') { - $options['proxy'] = $proxyUri; + + $options = array_merge($defaults, $options); + + 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) { - $proxyUri .= $proxyUserPwd . '@'; - } - if ($proxyHost !== null) { - $proxyUri .= $proxyHost; + if ($proxyUserPwd === null) { + return $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, 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 +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, array_merge($this->getRequestOptions(), $options)); + $response = $this->client->request('head', $uri, $this->buildRequestOptions($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, array_merge($this->getRequestOptions(), $options)); + $response = $this->client->request('put', $uri, $this->buildRequestOptions($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, array_merge($this->getRequestOptions(), $options)); + $response = $this->client->request('delete', $uri, $this->buildRequestOptions($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, 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 fa8544f07a..1df54010a2 100644 --- 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 1bfaf05035..02f331483d 100644 --- 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 7f12a824d1..f43958ab86 100644 --- 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,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, - HandlerStack::create() + $this->guzzleClient ); } - public function testGetProxyUri() { + public function testGetProxyUri(): void { $this->config ->expects($this->at(0)) ->method('getSystemValue') ->with('proxy', null) ->willReturn(null); - $this->config - ->expects($this->at(1)) - ->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 +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' - ], 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 +301,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', [[]])); } }