Removed use of HttpFetcher

- Fixed some existing issues in new aligned process.
- Manually tested each external call scenario.

diff --git a/app/Activity/DispatchWebhookJob.php b/app/Activity/DispatchWebhookJob.php
index 09fa12785ea189949058f25132cefe175ed329f1..e1771b114cf4a5364be84d2e66bcba1c6767aad8 100644 (file)
--- 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 Illuminate\Queue\InteractsWithQueue;
 use Illuminate\Queue\SerializesModels;
 use Illuminate\Support\Facades\Log;

-use Psr\Http\Client\ClientExceptionInterface;

 
 class DispatchWebhookJob implements ShouldQueue
 {
 
 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}");
             }
                 $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}\"");
         }
             $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 7d019d77ca63b46cf1c8dbf58634864af6f6a1c6..f224b177997d88ef9d531316b847a5310bc0ab37 100644 (file)
--- a/app/Http/HttpClientHistory.php
+++ b/app/Http/HttpClientHistory.php
@@ -25,4 +25,9 @@ class HttpClientHistory
     {
         return $this->requestAt($this->requestCount() - 1);
     }
     {
         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 8318474aa9cc97b03698f716b7e9684c4f657fc5..f59c298a62b8e2596f422c0a0d86766115bec8ac 100644 (file)
--- 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\HandlerStack;
 use GuzzleHttp\Middleware;
 use GuzzleHttp\Psr7\Request as GuzzleRequest;

+use GuzzleHttp\Psr7\Response;

 use Psr\Http\Client\ClientInterface;
 
 class HttpRequestService
 use Psr\Http\Client\ClientInterface;
 
 class HttpRequestService


@@ -16,7 +17,7 @@ class HttpRequestService
     /**
      * Build a new http client for sending requests on.
      */
     /**
      * 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,
     {
         $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
      */
      * 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);
         $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 (file)
index fcb4147..0000000
--- a/app/Uploads/HttpFetcher.php
+++ /dev/null
@@ -1,38 +0,0 @@
-<?php
-
-namespace BookStack\Uploads;
-
-use BookStack\Exceptions\HttpFetchException;
-
-class HttpFetcher
-{
-    /**
-     * Fetch content from an external URI.
-     *
-     * @param string $uri
-     *
-     * @throws HttpFetchException
-     *
-     * @return bool|string
-     */
-    public function fetch(string $uri)
-    {
-        $ch = curl_init();
-        curl_setopt_array($ch, [
-            CURLOPT_URL            => $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 3cd37812acbd40f14aec23ae2bb1d386a49df693..9692b3f38aff75936355c2c57143727c16067b80 100644 (file)
--- a/app/Uploads/UserAvatars.php
+++ b/app/Uploads/UserAvatars.php
@@ -3,20 +3,20 @@
 namespace BookStack\Uploads;
 
 use BookStack\Exceptions\HttpFetchException;
 namespace BookStack\Uploads;
 
 use BookStack\Exceptions\HttpFetchException;

+use BookStack\Http\HttpRequestService;

 use BookStack\Users\Models\User;
 use Exception;
 use BookStack\Users\Models\User;
 use Exception;

+use GuzzleHttp\Psr7\Request;

 use Illuminate\Support\Facades\Log;
 use Illuminate\Support\Str;
 use Illuminate\Support\Facades\Log;
 use Illuminate\Support\Str;

+use Psr\Http\Client\ClientExceptionInterface;

 
 class UserAvatars
 {
 
 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 {
     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);
         }
 
             throw new HttpFetchException(trans('errors.cannot_get_image_from_url', ['url' => $url]), $exception->getCode(), $exception);
         }
 


@@ -127,7 +129,7 @@ class UserAvatars
     {
         $fetchUrl = $this->getAvatarUrl();
 
     {
         $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 e3c47cd89588d0370be487db5c114ececc56a7dc..f8f59977a1e1ac6a282f91e0cca4399e0c47b853 100644 (file)
--- 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\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;
 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 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;
 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.
      */
     /**
      * Mock the http client used in BookStack http calls.
      */

diff --git a/tests/Uploads/AvatarTest.php b/tests/Uploads/AvatarTest.php
index 363c1fa95420b858d42980f27e05d8e38e10a07a..f5b49a9fc21c245288b19cbd7966637b7dbb6bec 100644 (file)
--- a/tests/Uploads/AvatarTest.php
+++ b/tests/Uploads/AvatarTest.php
@@ -3,9 +3,11 @@
 namespace Tests\Uploads;
 
 use BookStack\Exceptions\HttpFetchException;
 namespace Tests\Uploads;
 
 use BookStack\Exceptions\HttpFetchException;

-use BookStack\Uploads\HttpFetcher;

 use BookStack\Uploads\UserAvatars;
 use BookStack\Users\Models\User;
 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
 use Tests\TestCase;
 
 class AvatarTest extends TestCase


@@ -22,27 +24,16 @@ class AvatarTest extends TestCase
         return User::query()->where('email', '=', $user->email)->first();
     }
 
         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()
     {
     {
         $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();
         $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', [
 
         $user = $this->createUserRequest($user);
         $this->assertDatabaseHas('images', [


@@ -50,6 +41,9 @@ class AvatarTest extends TestCase
             'created_by' => $user->id,
         ]);
         $this->deleteUserImage($user);
             '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()
     }
 
     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';
 
         $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);
 
         $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()
     {
         $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();
         $user = User::factory()->make();

-
-        $http = $this->mock(HttpFetcher::class);
-        $http->shouldNotReceive('fetch');
+        $requests = $this->mockHttpClient([new Response()]);

 
         $this->createUserRequest($user);
 
         $this->createUserRequest($user);

+
+        $this->assertEquals(0, $requests->requestCount());

     }
 
     public function test_avatar_not_fetched_if_avatar_url_option_set_to_false()
     }
 
     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();
         ]);
 
         $user = User::factory()->make();

-
-        $http = $this->mock(HttpFetcher::class);
-        $http->shouldNotReceive('fetch');
+        $requests = $this->mockHttpClient([new Response()]);

 
         $this->createUserRequest($user);
 
         $this->createUserRequest($user);

+
+        $this->assertEquals(0, $requests->requestCount());

     }
 
     public function test_no_failure_but_error_logged_on_failed_avatar_fetch()
     {
     }
 
     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();
 
 
         $logger = $this->withTestLogger();
 


@@ -122,17 +111,16 @@ class AvatarTest extends TestCase
 
         $user = User::factory()->make();
         $avatar = app()->make(UserAvatars::class);
 
         $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();
         $logger = $this->withTestLogger();

+        $this->mockHttpClient([new ConnectException('Could not resolve host http_malformed_url', new Request('GET', ''))]);

 
         $avatar->fetchAndAssignToUser($user);
 
 
         $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->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());

     }
 }
     }
 }