From 06490f624c3923e945bf86a2930ff85c062a0bad Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 8 Sep 2023 17:16:57 +0100 Subject: [PATCH] Removed use of HttpFetcher - Fixed some existing issues in new aligned process. - Manually tested each external call scenario. --- app/Activity/DispatchWebhookJob.php | 3 +- app/Http/HttpClientHistory.php | 5 +++ app/Http/HttpRequestService.php | 13 +++++- app/Uploads/HttpFetcher.php | 38 ------------------ app/Uploads/UserAvatars.php | 22 +++++----- tests/TestCase.php | 15 ------- tests/Uploads/AvatarTest.php | 62 ++++++++++++----------------- 7 files changed, 54 insertions(+), 104 deletions(-) delete mode 100644 app/Uploads/HttpFetcher.php diff --git a/app/Activity/DispatchWebhookJob.php b/app/Activity/DispatchWebhookJob.php index 09fa12785..e1771b114 100644 --- a/app/Activity/DispatchWebhookJob.php +++ b/app/Activity/DispatchWebhookJob.php @@ -16,7 +16,6 @@ use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; use Illuminate\Support\Facades\Log; -use Psr\Http\Client\ClientExceptionInterface; class DispatchWebhookJob implements ShouldQueue { @@ -69,7 +68,7 @@ class DispatchWebhookJob implements ShouldQueue $lastError = "Response status from endpoint was {$statusCode}"; Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with status {$statusCode}"); } - } catch (ClientExceptionInterface $error) { + } catch (\Exception $error) { $lastError = $error->getMessage(); Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with error \"{$lastError}\""); } diff --git a/app/Http/HttpClientHistory.php b/app/Http/HttpClientHistory.php index 7d019d77c..f224b1779 100644 --- a/app/Http/HttpClientHistory.php +++ b/app/Http/HttpClientHistory.php @@ -25,4 +25,9 @@ class HttpClientHistory { return $this->requestAt($this->requestCount() - 1); } + + public function all(): array + { + return $this->container; + } } diff --git a/app/Http/HttpRequestService.php b/app/Http/HttpRequestService.php index 8318474aa..f59c298a6 100644 --- a/app/Http/HttpRequestService.php +++ b/app/Http/HttpRequestService.php @@ -7,6 +7,7 @@ use GuzzleHttp\Handler\MockHandler; use GuzzleHttp\HandlerStack; use GuzzleHttp\Middleware; use GuzzleHttp\Psr7\Request as GuzzleRequest; +use GuzzleHttp\Psr7\Response; use Psr\Http\Client\ClientInterface; class HttpRequestService @@ -16,7 +17,7 @@ class HttpRequestService /** * Build a new http client for sending requests on. */ - public function buildClient(int $timeout, array $options): ClientInterface + public function buildClient(int $timeout, array $options = []): ClientInterface { $defaultOptions = [ 'timeout' => $timeout, @@ -40,8 +41,16 @@ class HttpRequestService * Returns history which can then be queried. * @link https://docs.guzzlephp.org/en/stable/testing.html#history-middleware */ - public function mockClient(array $responses = []): HttpClientHistory + public function mockClient(array $responses = [], bool $pad = true): HttpClientHistory { + // By default, we pad out the responses with 10 successful values so that requests will be + // properly recorded for inspection. Otherwise, we can't later check if we're received + // too many requests. + if ($pad) { + $response = new Response(200, [], 'success'); + $responses = array_merge($responses, array_fill(0, 10, $response)); + } + $container = []; $history = Middleware::history($container); $mock = new MockHandler($responses); diff --git a/app/Uploads/HttpFetcher.php b/app/Uploads/HttpFetcher.php deleted file mode 100644 index fcb4147e9..000000000 --- a/app/Uploads/HttpFetcher.php +++ /dev/null @@ -1,38 +0,0 @@ - $uri, - CURLOPT_RETURNTRANSFER => 1, - CURLOPT_CONNECTTIMEOUT => 5, - ]); - - $data = curl_exec($ch); - $err = curl_error($ch); - curl_close($ch); - - if ($err) { - $errno = curl_errno($ch); - throw new HttpFetchException($err, $errno); - } - - return $data; - } -} diff --git a/app/Uploads/UserAvatars.php b/app/Uploads/UserAvatars.php index 3cd37812a..9692b3f38 100644 --- a/app/Uploads/UserAvatars.php +++ b/app/Uploads/UserAvatars.php @@ -3,20 +3,20 @@ namespace BookStack\Uploads; use BookStack\Exceptions\HttpFetchException; +use BookStack\Http\HttpRequestService; use BookStack\Users\Models\User; use Exception; +use GuzzleHttp\Psr7\Request; use Illuminate\Support\Facades\Log; use Illuminate\Support\Str; +use Psr\Http\Client\ClientExceptionInterface; class UserAvatars { - protected $imageService; - protected $http; - - public function __construct(ImageService $imageService, HttpFetcher $http) - { - $this->imageService = $imageService; - $this->http = $http; + public function __construct( + protected ImageService $imageService, + protected HttpRequestService $http + ) { } /** @@ -112,8 +112,10 @@ class UserAvatars protected function getAvatarImageData(string $url): string { try { - $imageData = $this->http->fetch($url); - } catch (HttpFetchException $exception) { + $client = $this->http->buildClient(5); + $response = $client->sendRequest(new Request('GET', $url)); + $imageData = (string) $response->getBody(); + } catch (ClientExceptionInterface $exception) { throw new HttpFetchException(trans('errors.cannot_get_image_from_url', ['url' => $url]), $exception->getCode(), $exception); } @@ -127,7 +129,7 @@ class UserAvatars { $fetchUrl = $this->getAvatarUrl(); - return is_string($fetchUrl) && strpos($fetchUrl, 'http') === 0; + return str_starts_with($fetchUrl, 'http'); } /** diff --git a/tests/TestCase.php b/tests/TestCase.php index e3c47cd89..f8f59977a 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -6,7 +6,6 @@ use BookStack\Entities\Models\Entity; use BookStack\Http\HttpClientHistory; use BookStack\Http\HttpRequestService; use BookStack\Settings\SettingService; -use BookStack\Uploads\HttpFetcher; use BookStack\Users\Models\User; use Illuminate\Contracts\Console\Kernel; use Illuminate\Foundation\Testing\DatabaseTransactions; @@ -16,7 +15,6 @@ use Illuminate\Support\Env; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; use Illuminate\Testing\Assert as PHPUnit; -use Mockery; use Monolog\Handler\TestHandler; use Monolog\Logger; use Ssddanbrown\AssertHtml\TestsHtml; @@ -107,19 +105,6 @@ abstract class TestCase extends BaseTestCase } } - /** - * Mock the HttpFetcher service and return the given data on fetch. - */ - protected function mockHttpFetch($returnData, int $times = 1) - { - // TODO - Remove - $mockHttp = Mockery::mock(HttpFetcher::class); - $this->app[HttpFetcher::class] = $mockHttp; - $mockHttp->shouldReceive('fetch') - ->times($times) - ->andReturn($returnData); - } - /** * Mock the http client used in BookStack http calls. */ diff --git a/tests/Uploads/AvatarTest.php b/tests/Uploads/AvatarTest.php index 363c1fa95..f5b49a9fc 100644 --- a/tests/Uploads/AvatarTest.php +++ b/tests/Uploads/AvatarTest.php @@ -3,9 +3,11 @@ namespace Tests\Uploads; use BookStack\Exceptions\HttpFetchException; -use BookStack\Uploads\HttpFetcher; use BookStack\Uploads\UserAvatars; use BookStack\Users\Models\User; +use GuzzleHttp\Exception\ConnectException; +use GuzzleHttp\Psr7\Request; +use GuzzleHttp\Psr7\Response; use Tests\TestCase; class AvatarTest extends TestCase @@ -22,27 +24,16 @@ class AvatarTest extends TestCase return User::query()->where('email', '=', $user->email)->first(); } - protected function assertImageFetchFrom(string $url) - { - $http = $this->mock(HttpFetcher::class); - - $http->shouldReceive('fetch') - ->once()->with($url) - ->andReturn($this->files->pngImageData()); - } - - protected function deleteUserImage(User $user) + protected function deleteUserImage(User $user): void { $this->files->deleteAtRelativePath($user->avatar->path); } public function test_gravatar_fetched_on_user_create() { - config()->set([ - 'services.disable_services' => false, - ]); + $requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]); + config()->set(['services.disable_services' => false]); $user = User::factory()->make(); - $this->assertImageFetchFrom('https://www.gravatar.com/avatar/' . md5(strtolower($user->email)) . '?s=500&d=identicon'); $user = $this->createUserRequest($user); $this->assertDatabaseHas('images', [ @@ -50,6 +41,9 @@ class AvatarTest extends TestCase 'created_by' => $user->id, ]); $this->deleteUserImage($user); + + $expectedUri = 'https://www.gravatar.com/avatar/' . md5(strtolower($user->email)) . '?s=500&d=identicon'; + $this->assertEquals($expectedUri, $requests->latestRequest()->getUri()); } public function test_custom_url_used_if_set() @@ -61,24 +55,22 @@ class AvatarTest extends TestCase $user = User::factory()->make(); $url = 'https://example.com/' . urlencode(strtolower($user->email)) . '/' . md5(strtolower($user->email)) . '/500'; - $this->assertImageFetchFrom($url); + $requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]); $user = $this->createUserRequest($user); + $this->assertEquals($url, $requests->latestRequest()->getUri()); $this->deleteUserImage($user); } public function test_avatar_not_fetched_if_no_custom_url_and_services_disabled() { - config()->set([ - 'services.disable_services' => true, - ]); - + config()->set(['services.disable_services' => true]); $user = User::factory()->make(); - - $http = $this->mock(HttpFetcher::class); - $http->shouldNotReceive('fetch'); + $requests = $this->mockHttpClient([new Response()]); $this->createUserRequest($user); + + $this->assertEquals(0, $requests->requestCount()); } public function test_avatar_not_fetched_if_avatar_url_option_set_to_false() @@ -89,21 +81,18 @@ class AvatarTest extends TestCase ]); $user = User::factory()->make(); - - $http = $this->mock(HttpFetcher::class); - $http->shouldNotReceive('fetch'); + $requests = $this->mockHttpClient([new Response()]); $this->createUserRequest($user); + + $this->assertEquals(0, $requests->requestCount()); } public function test_no_failure_but_error_logged_on_failed_avatar_fetch() { - config()->set([ - 'services.disable_services' => false, - ]); + config()->set(['services.disable_services' => false]); - $http = $this->mock(HttpFetcher::class); - $http->shouldReceive('fetch')->andThrow(new HttpFetchException()); + $this->mockHttpClient([new ConnectException('Failed to connect', new Request('GET', ''))]); $logger = $this->withTestLogger(); @@ -122,17 +111,16 @@ class AvatarTest extends TestCase $user = User::factory()->make(); $avatar = app()->make(UserAvatars::class); - $url = 'http_malformed_url/' . urlencode(strtolower($user->email)) . '/' . md5(strtolower($user->email)) . '/500'; $logger = $this->withTestLogger(); + $this->mockHttpClient([new ConnectException('Could not resolve host http_malformed_url', new Request('GET', ''))]); $avatar->fetchAndAssignToUser($user); + $url = 'http_malformed_url/' . urlencode(strtolower($user->email)) . '/' . md5(strtolower($user->email)) . '/500'; $this->assertTrue($logger->hasError('Failed to save user avatar image')); $exception = $logger->getRecords()[0]['context']['exception']; - $this->assertEquals(new HttpFetchException( - 'Cannot get image from ' . $url, - 6, - (new HttpFetchException('Could not resolve host: http_malformed_url', 6)) - ), $exception); + $this->assertInstanceOf(HttpFetchException::class, $exception); + $this->assertEquals('Cannot get image from ' . $url, $exception->getMessage()); + $this->assertEquals('Could not resolve host http_malformed_url', $exception->getPrevious()->getMessage()); } }