mirror of
https://github.com/BookStackApp/BookStack.git
synced 2024-10-29 23:22:34 +01:00
Removed use of HttpFetcher
- Fixed some existing issues in new aligned process. - Manually tested each external call scenario.
This commit is contained in:
parent
a8b5652210
commit
06490f624c
@ -16,7 +16,6 @@ use Illuminate\Foundation\Bus\Dispatchable;
|
|||||||
use Illuminate\Queue\InteractsWithQueue;
|
use Illuminate\Queue\InteractsWithQueue;
|
||||||
use Illuminate\Queue\SerializesModels;
|
use Illuminate\Queue\SerializesModels;
|
||||||
use Illuminate\Support\Facades\Log;
|
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}";
|
$lastError = "Response status from endpoint was {$statusCode}";
|
||||||
Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with status {$statusCode}");
|
Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with status {$statusCode}");
|
||||||
}
|
}
|
||||||
} catch (ClientExceptionInterface $error) {
|
} catch (\Exception $error) {
|
||||||
$lastError = $error->getMessage();
|
$lastError = $error->getMessage();
|
||||||
Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with error \"{$lastError}\"");
|
Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with error \"{$lastError}\"");
|
||||||
}
|
}
|
||||||
|
@ -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;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
@ -7,6 +7,7 @@ use GuzzleHttp\Handler\MockHandler;
|
|||||||
use GuzzleHttp\HandlerStack;
|
use GuzzleHttp\HandlerStack;
|
||||||
use GuzzleHttp\Middleware;
|
use GuzzleHttp\Middleware;
|
||||||
use GuzzleHttp\Psr7\Request as GuzzleRequest;
|
use GuzzleHttp\Psr7\Request as GuzzleRequest;
|
||||||
|
use GuzzleHttp\Psr7\Response;
|
||||||
use Psr\Http\Client\ClientInterface;
|
use Psr\Http\Client\ClientInterface;
|
||||||
|
|
||||||
class HttpRequestService
|
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 = [
|
$defaultOptions = [
|
||||||
'timeout' => $timeout,
|
'timeout' => $timeout,
|
||||||
@ -40,8 +41,16 @@ class HttpRequestService
|
|||||||
* Returns history which can then be queried.
|
* Returns history which can then be queried.
|
||||||
* @link https://docs.guzzlephp.org/en/stable/testing.html#history-middleware
|
* @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 = [];
|
$container = [];
|
||||||
$history = Middleware::history($container);
|
$history = Middleware::history($container);
|
||||||
$mock = new MockHandler($responses);
|
$mock = new MockHandler($responses);
|
||||||
|
@ -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;
|
|
||||||
}
|
|
||||||
}
|
|
@ -3,20 +3,20 @@
|
|||||||
namespace BookStack\Uploads;
|
namespace BookStack\Uploads;
|
||||||
|
|
||||||
use BookStack\Exceptions\HttpFetchException;
|
use BookStack\Exceptions\HttpFetchException;
|
||||||
|
use BookStack\Http\HttpRequestService;
|
||||||
use BookStack\Users\Models\User;
|
use BookStack\Users\Models\User;
|
||||||
use Exception;
|
use Exception;
|
||||||
|
use GuzzleHttp\Psr7\Request;
|
||||||
use Illuminate\Support\Facades\Log;
|
use Illuminate\Support\Facades\Log;
|
||||||
use Illuminate\Support\Str;
|
use Illuminate\Support\Str;
|
||||||
|
use Psr\Http\Client\ClientExceptionInterface;
|
||||||
|
|
||||||
class UserAvatars
|
class UserAvatars
|
||||||
{
|
{
|
||||||
protected $imageService;
|
public function __construct(
|
||||||
protected $http;
|
protected ImageService $imageService,
|
||||||
|
protected HttpRequestService $http
|
||||||
public function __construct(ImageService $imageService, HttpFetcher $http)
|
) {
|
||||||
{
|
|
||||||
$this->imageService = $imageService;
|
|
||||||
$this->http = $http;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@ -112,8 +112,10 @@ class UserAvatars
|
|||||||
protected function getAvatarImageData(string $url): string
|
protected function getAvatarImageData(string $url): string
|
||||||
{
|
{
|
||||||
try {
|
try {
|
||||||
$imageData = $this->http->fetch($url);
|
$client = $this->http->buildClient(5);
|
||||||
} catch (HttpFetchException $exception) {
|
$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');
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
@ -6,7 +6,6 @@ use BookStack\Entities\Models\Entity;
|
|||||||
use BookStack\Http\HttpClientHistory;
|
use BookStack\Http\HttpClientHistory;
|
||||||
use BookStack\Http\HttpRequestService;
|
use BookStack\Http\HttpRequestService;
|
||||||
use BookStack\Settings\SettingService;
|
use BookStack\Settings\SettingService;
|
||||||
use BookStack\Uploads\HttpFetcher;
|
|
||||||
use BookStack\Users\Models\User;
|
use BookStack\Users\Models\User;
|
||||||
use Illuminate\Contracts\Console\Kernel;
|
use Illuminate\Contracts\Console\Kernel;
|
||||||
use Illuminate\Foundation\Testing\DatabaseTransactions;
|
use Illuminate\Foundation\Testing\DatabaseTransactions;
|
||||||
@ -16,7 +15,6 @@ use Illuminate\Support\Env;
|
|||||||
use Illuminate\Support\Facades\DB;
|
use Illuminate\Support\Facades\DB;
|
||||||
use Illuminate\Support\Facades\Log;
|
use Illuminate\Support\Facades\Log;
|
||||||
use Illuminate\Testing\Assert as PHPUnit;
|
use Illuminate\Testing\Assert as PHPUnit;
|
||||||
use Mockery;
|
|
||||||
use Monolog\Handler\TestHandler;
|
use Monolog\Handler\TestHandler;
|
||||||
use Monolog\Logger;
|
use Monolog\Logger;
|
||||||
use Ssddanbrown\AssertHtml\TestsHtml;
|
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.
|
||||||
*/
|
*/
|
||||||
|
@ -3,9 +3,11 @@
|
|||||||
namespace Tests\Uploads;
|
namespace Tests\Uploads;
|
||||||
|
|
||||||
use BookStack\Exceptions\HttpFetchException;
|
use BookStack\Exceptions\HttpFetchException;
|
||||||
use BookStack\Uploads\HttpFetcher;
|
|
||||||
use BookStack\Uploads\UserAvatars;
|
use BookStack\Uploads\UserAvatars;
|
||||||
use BookStack\Users\Models\User;
|
use BookStack\Users\Models\User;
|
||||||
|
use GuzzleHttp\Exception\ConnectException;
|
||||||
|
use GuzzleHttp\Psr7\Request;
|
||||||
|
use GuzzleHttp\Psr7\Response;
|
||||||
use Tests\TestCase;
|
use Tests\TestCase;
|
||||||
|
|
||||||
class AvatarTest extends 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)
|
protected function deleteUserImage(User $user): void
|
||||||
{
|
|
||||||
$http = $this->mock(HttpFetcher::class);
|
|
||||||
|
|
||||||
$http->shouldReceive('fetch')
|
|
||||||
->once()->with($url)
|
|
||||||
->andReturn($this->files->pngImageData());
|
|
||||||
}
|
|
||||||
|
|
||||||
protected function deleteUserImage(User $user)
|
|
||||||
{
|
{
|
||||||
$this->files->deleteAtRelativePath($user->avatar->path);
|
$this->files->deleteAtRelativePath($user->avatar->path);
|
||||||
}
|
}
|
||||||
|
|
||||||
public function test_gravatar_fetched_on_user_create()
|
public function test_gravatar_fetched_on_user_create()
|
||||||
{
|
{
|
||||||
config()->set([
|
$requests = $this->mockHttpClient([new Response(200, ['Content-Type' => 'image/png'], $this->files->pngImageData())]);
|
||||||
'services.disable_services' => false,
|
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);
|
$user = $this->createUserRequest($user);
|
||||||
$this->assertDatabaseHas('images', [
|
$this->assertDatabaseHas('images', [
|
||||||
@ -50,6 +41,9 @@ class AvatarTest extends TestCase
|
|||||||
'created_by' => $user->id,
|
'created_by' => $user->id,
|
||||||
]);
|
]);
|
||||||
$this->deleteUserImage($user);
|
$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();
|
$user = User::factory()->make();
|
||||||
$url = 'https://example.com/' . urlencode(strtolower($user->email)) . '/' . md5(strtolower($user->email)) . '/500';
|
$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);
|
$this->deleteUserImage($user);
|
||||||
}
|
}
|
||||||
|
|
||||||
public function test_avatar_not_fetched_if_no_custom_url_and_services_disabled()
|
public function test_avatar_not_fetched_if_no_custom_url_and_services_disabled()
|
||||||
{
|
{
|
||||||
config()->set([
|
config()->set(['services.disable_services' => true]);
|
||||||
'services.disable_services' => true,
|
|
||||||
]);
|
|
||||||
|
|
||||||
$user = User::factory()->make();
|
$user = User::factory()->make();
|
||||||
|
$requests = $this->mockHttpClient([new Response()]);
|
||||||
$http = $this->mock(HttpFetcher::class);
|
|
||||||
$http->shouldNotReceive('fetch');
|
|
||||||
|
|
||||||
$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();
|
||||||
|
$requests = $this->mockHttpClient([new Response()]);
|
||||||
$http = $this->mock(HttpFetcher::class);
|
|
||||||
$http->shouldNotReceive('fetch');
|
|
||||||
|
|
||||||
$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([
|
config()->set(['services.disable_services' => false]);
|
||||||
'services.disable_services' => false,
|
|
||||||
]);
|
|
||||||
|
|
||||||
$http = $this->mock(HttpFetcher::class);
|
$this->mockHttpClient([new ConnectException('Failed to connect', new Request('GET', ''))]);
|
||||||
$http->shouldReceive('fetch')->andThrow(new HttpFetchException());
|
|
||||||
|
|
||||||
$logger = $this->withTestLogger();
|
$logger = $this->withTestLogger();
|
||||||
|
|
||||||
@ -122,17 +111,16 @@ class AvatarTest extends TestCase
|
|||||||
|
|
||||||
$user = User::factory()->make();
|
$user = User::factory()->make();
|
||||||
$avatar = app()->make(UserAvatars::class);
|
$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'));
|
$this->assertTrue($logger->hasError('Failed to save user avatar image'));
|
||||||
$exception = $logger->getRecords()[0]['context']['exception'];
|
$exception = $logger->getRecords()[0]['context']['exception'];
|
||||||
$this->assertEquals(new HttpFetchException(
|
$this->assertInstanceOf(HttpFetchException::class, $exception);
|
||||||
'Cannot get image from ' . $url,
|
$this->assertEquals('Cannot get image from ' . $url, $exception->getMessage());
|
||||||
6,
|
$this->assertEquals('Could not resolve host http_malformed_url', $exception->getPrevious()->getMessage());
|
||||||
(new HttpFetchException('Could not resolve host: http_malformed_url', 6))
|
|
||||||
), $exception);
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
Loading…
Reference in New Issue
Block a user