github-mirror/BookStack
2
0
Fork 0
mirror of https://github.com/BookStackApp/BookStack.git synced 2024-11-22 12:55:14 +08:00
Code Issues Actions Packages Projects Releases Wiki Activity

Started aligning app-wide outbound http calling behaviour

Browse Source
This commit is contained in:
Dan Brown 2023-09-08 14:16:09 +01:00
parent 21cd2d17f6
commit a8b5652210
No known key found for this signature in database
GPG Key ID: 46D9F943C24A2EF9
11 changed files with 159 additions and 107 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
app/Access/Oidc/OidcOAuthProvider.php
View File

@ -20,15 +20,8 @@ class OidcOAuthProvider extends AbstractProvider
{
use BearerAuthorizationTrait;
/**
* @var string
*/
protected $authorizationEndpoint;
/**
* @var string
*/
protected $tokenEndpoint;
protected string $authorizationEndpoint;
protected string $tokenEndpoint;
/**
* Scopes to use for the OIDC authorization call.
@ -60,7 +53,7 @@ class OidcOAuthProvider extends AbstractProvider
}
/**
* Add an additional scope to this provider upon the default.
* Add another scope to this provider upon the default.
*/
public function addScope(string $scope): void
{

2
app/Access/Oidc/OidcProviderSettings.php
View File

@ -59,7 +59,7 @@ class OidcProviderSettings
}
}
if (strpos($this->issuer, 'https://') !== 0) {
if (!str_starts_with($this->issuer, 'https://')) {
throw new InvalidArgumentException('Issuer value must start with https://');
}
}

8
app/Access/Oidc/OidcService.php
View File

@ -9,13 +9,13 @@ use BookStack\Exceptions\JsonDebugException;
use BookStack\Exceptions\StoppedAuthenticationException;
use BookStack\Exceptions\UserRegistrationException;
use BookStack\Facades\Theme;
use BookStack\Http\HttpRequestService;
use BookStack\Theming\ThemeEvents;
use BookStack\Users\Models\User;
use Illuminate\Support\Arr;
use Illuminate\Support\Facades\Cache;
use League\OAuth2\Client\OptionProvider\HttpBasicAuthOptionProvider;
use League\OAuth2\Client\Provider\Exception\IdentityProviderException;
use Psr\Http\Client\ClientInterface as HttpClient;
/**
* Class OpenIdConnectService
@ -26,7 +26,7 @@ class OidcService
public function __construct(
protected RegistrationService $registrationService,
protected LoginService $loginService,
protected HttpClient $httpClient,
protected HttpRequestService $http,
protected GroupSyncService $groupService
) {
}
@ -94,7 +94,7 @@ class OidcService
// Run discovery
if ($config['discover'] ?? false) {
try {
$settings->discoverFromIssuer($this->httpClient, Cache::store(null), 15);
$settings->discoverFromIssuer($this->http->buildClient(5), Cache::store(null), 15);
} catch (OidcIssuerDiscoveryException $exception) {
throw new OidcException('OIDC Discovery Error: ' . $exception->getMessage());
}
@ -111,7 +111,7 @@ class OidcService
protected function getProvider(OidcProviderSettings $settings): OidcOAuthProvider
{
$provider = new OidcOAuthProvider($settings->arrayForProvider(), [
'httpClient' => $this->httpClient,
'httpClient' => $this->http->buildClient(5),
'optionProvider' => new HttpBasicAuthOptionProvider(),
]);

30
app/Activity/DispatchWebhookJob.php
View File

@ -6,6 +6,7 @@ use BookStack\Activity\Models\Loggable;
use BookStack\Activity\Models\Webhook;
use BookStack\Activity\Tools\WebhookFormatter;
use BookStack\Facades\Theme;
use BookStack\Http\HttpRequestService;
use BookStack\Theming\ThemeEvents;
use BookStack\Users\Models\User;
use BookStack\Util\SsrUrlValidator;
@ -14,8 +15,8 @@ use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
use Illuminate\Queue\InteractsWithQueue;
use Illuminate\Queue\SerializesModels;
use Illuminate\Support\Facades\Http;
use Illuminate\Support\Facades\Log;
use Psr\Http\Client\ClientExceptionInterface;
class DispatchWebhookJob implements ShouldQueue
{
@ -49,25 +50,28 @@ class DispatchWebhookJob implements ShouldQueue
*
* @return void
*/
public function handle()
public function handle(HttpRequestService $http)
{
$lastError = null;
try {
(new SsrUrlValidator())->ensureAllowed($this->webhook->endpoint);
$response = Http::asJson()
->withOptions(['allow_redirects' => ['strict' => true]])
->timeout($this->webhook->timeout)
->post($this->webhook->endpoint, $this->webhookData);
} catch (\Exception $exception) {
$lastError = $exception->getMessage();
Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with error \"{$lastError}\"");
}
$client = $http->buildClient($this->webhook->timeout, [
'connect_timeout' => 10,
'allow_redirects' => ['strict' => true],
]);
if (isset($response) && $response->failed()) {
$lastError = "Response status from endpoint was {$response->status()}";
Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with status {$response->status()}");
$response = $client->sendRequest($http->jsonRequest('POST', $this->webhook->endpoint, $this->webhookData));
$statusCode = $response->getStatusCode();
if ($statusCode >= 400) {
$lastError = "Response status from endpoint was {$statusCode}";
Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with status {$statusCode}");
}
} catch (ClientExceptionInterface $error) {
$lastError = $error->getMessage();
Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with error \"{$lastError}\"");
}
$this->webhook->last_called_at = now();

12
app/App/Providers/AppServiceProvider.php
View File

@ -9,16 +9,15 @@ use BookStack\Entities\Models\Bookshelf;
use BookStack\Entities\Models\Chapter;
use BookStack\Entities\Models\Page;
use BookStack\Exceptions\BookStackExceptionHandlerPage;
use BookStack\Http\HttpRequestService;
use BookStack\Permissions\PermissionApplicator;
use BookStack\Settings\SettingService;
use BookStack\Util\CspService;
use GuzzleHttp\Client;
use Illuminate\Contracts\Foundation\ExceptionRenderer;
use Illuminate\Database\Eloquent\Relations\Relation;
use Illuminate\Support\Facades\Schema;
use Illuminate\Support\Facades\URL;
use Illuminate\Support\ServiceProvider;
use Psr\Http\Client\ClientInterface as HttpClientInterface;
class AppServiceProvider extends ServiceProvider
{
@ -39,6 +38,7 @@ class AppServiceProvider extends ServiceProvider
SettingService::class => SettingService::class,
SocialAuthService::class => SocialAuthService::class,
CspService::class => CspService::class,
HttpRequestService::class => HttpRequestService::class,
];
/**
@ -51,7 +51,7 @@ class AppServiceProvider extends ServiceProvider
// Set root URL
$appUrl = config('app.url');
if ($appUrl) {
$isHttps = (strpos($appUrl, 'https://') === 0);
$isHttps = str_starts_with($appUrl, 'https://');
URL::forceRootUrl($appUrl);
URL::forceScheme($isHttps ? 'https' : 'http');
}
@ -75,12 +75,6 @@ class AppServiceProvider extends ServiceProvider
*/
public function register()
{
$this->app->bind(HttpClientInterface::class, function ($app) {
return new Client([
'timeout' => 3,
]);
});
$this->app->singleton(PermissionApplicator::class, function ($app) {
return new PermissionApplicator(null);
});

28
app/Http/HttpClientHistory.php Normal file
View File

@ -0,0 +1,28 @@
<?php
namespace BookStack\Http;
use GuzzleHttp\Psr7\Request as GuzzleRequest;
class HttpClientHistory
{
public function __construct(
protected &$container
) {
}
public function requestCount(): int
{
return count($this->container);
}
public function requestAt(int $index): ?GuzzleRequest
{
return $this->container[$index]['request'] ?? null;
}
public function latestRequest(): ?GuzzleRequest
{
return $this->requestAt($this->requestCount() - 1);
}
}

61
app/Http/HttpRequestService.php Normal file
View File

@ -0,0 +1,61 @@
<?php
namespace BookStack\Http;
use GuzzleHttp\Client;
use GuzzleHttp\Handler\MockHandler;
use GuzzleHttp\HandlerStack;
use GuzzleHttp\Middleware;
use GuzzleHttp\Psr7\Request as GuzzleRequest;
use Psr\Http\Client\ClientInterface;
class HttpRequestService
{
protected ?HandlerStack $handler = null;
/**
* Build a new http client for sending requests on.
*/
public function buildClient(int $timeout, array $options): ClientInterface
{
$defaultOptions = [
'timeout' => $timeout,
'handler' => $this->handler,
];
return new Client(array_merge($options, $defaultOptions));
}
/**
* Create a new JSON http request for use with a client.
*/
public function jsonRequest(string $method, string $uri, array $data): GuzzleRequest
{
$headers = ['Content-Type' => 'application/json'];
return new GuzzleRequest($method, $uri, $headers, json_encode($data));
}
/**
* Mock any http clients built from this service, and response with the given responses.
* Returns history which can then be queried.
* @link https://docs.guzzlephp.org/en/stable/testing.html#history-middleware
*/
public function mockClient(array $responses = []): HttpClientHistory
{
$container = [];
$history = Middleware::history($container);
$mock = new MockHandler($responses);
$this->handler = HandlerStack::create($mock);
$this->handler->push($history, 'history');
return new HttpClientHistory($container);
}
/**
* Clear mocking that has been set up for clients.
*/
public function clearMocking(): void
{
$this->handler = null;
}
}

50
tests/Actions/WebhookCallTest.php
View File

@ -7,11 +7,10 @@ use BookStack\Activity\DispatchWebhookJob;
use BookStack\Activity\Models\Webhook;
use BookStack\Activity\Tools\ActivityLogger;
use BookStack\Api\ApiToken;
use BookStack\Entities\Models\PageRevision;
use BookStack\Users\Models\User;
use Illuminate\Http\Client\Request;
use GuzzleHttp\Exception\ConnectException;
use GuzzleHttp\Psr7\Response;
use Illuminate\Support\Facades\Bus;
use Illuminate\Support\Facades\Http;
use Tests\TestCase;
class WebhookCallTest extends TestCase
@ -50,10 +49,10 @@ class WebhookCallTest extends TestCase
public function test_webhook_runs_for_delete_actions()
{
// This test must not fake the queue/bus since this covers an issue
// around handling and serialization of items now deleted from the database.
$this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.com'], ['all']);
Http::fake([
'*' => Http::response('', 500),
]);
$this->mockHttpClient([new Response(500)]);
$user = $this->users->newUser();
$resp = $this->asAdmin()->delete($user->getEditUrl());
@ -69,9 +68,7 @@ class WebhookCallTest extends TestCase
public function test_failed_webhook_call_logs_error()
{
$logger = $this->withTestLogger();
Http::fake([
'*' => Http::response('', 500),
]);
$this->mockHttpClient([new Response(500)]);
$webhook = $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.com'], ['all']);
$this->assertNull($webhook->last_errored_at);
@ -86,7 +83,7 @@ class WebhookCallTest extends TestCase
public function test_webhook_call_exception_is_caught_and_logged()
{
Http::shouldReceive('asJson')->andThrow(new \Exception('Failed to perform request'));
$this->mockHttpClient([new ConnectException('Failed to perform request', new \GuzzleHttp\Psr7\Request('GET', ''))]);
$logger = $this->withTestLogger();
$webhook = $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.com'], ['all']);
@ -104,11 +101,11 @@ class WebhookCallTest extends TestCase
public function test_webhook_uses_ssr_hosts_option_if_set()
{
config()->set('app.ssr_hosts', 'https://*.example.com');
$http = Http::fake();
$responses = $this->mockHttpClient();
$webhook = $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.co.uk'], ['all']);
$this->runEvent(ActivityType::ROLE_CREATE);
$http->assertNothingSent();
$this->assertEquals(0, $responses->requestCount());
$webhook->refresh();
$this->assertEquals('The URL does not match the configured allowed SSR hosts', $webhook->last_error);
@ -117,29 +114,24 @@ class WebhookCallTest extends TestCase
public function test_webhook_call_data_format()
{
Http::fake([
'*' => Http::response('', 200),
]);
$responses = $this->mockHttpClient([new Response(200, [], '')]);
$webhook = $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.com'], ['all']);
$page = $this->entities->page();
$editor = $this->users->editor();
$this->runEvent(ActivityType::PAGE_UPDATE, $page, $editor);
Http::assertSent(function (Request $request) use ($editor, $page, $webhook) {
$reqData = $request->data();
return $request->isJson()
&& $reqData['event'] === 'page_update'
&& $reqData['text'] === ($editor->name . ' updated page "' . $page->name . '"')
&& is_string($reqData['triggered_at'])
&& $reqData['triggered_by']['name'] === $editor->name
&& $reqData['triggered_by_profile_url'] === $editor->getProfileUrl()
&& $reqData['webhook_id'] === $webhook->id
&& $reqData['webhook_name'] === $webhook->name
&& $reqData['url'] === $page->getUrl()
&& $reqData['related_item']['name'] === $page->name;
});
$request = $responses->latestRequest();
$reqData = json_decode($request->getBody(), true);
$this->assertEquals('page_update', $reqData['event']);
$this->assertEquals(($editor->name . ' updated page "' . $page->name . '"'), $reqData['text']);
$this->assertIsString($reqData['triggered_at']);
$this->assertEquals($editor->name, $reqData['triggered_by']['name']);
$this->assertEquals($editor->getProfileUrl(), $reqData['triggered_by_profile_url']);
$this->assertEquals($webhook->id, $reqData['webhook_id']);
$this->assertEquals($webhook->name, $reqData['webhook_name']);
$this->assertEquals($page->getUrl(), $reqData['url']);
$this->assertEquals($page->name, $reqData['related_item']['name']);
}
protected function runEvent(string $event, $detail = '', ?User $user = null)

24
tests/Auth/OidcTest.php
View File

@ -7,7 +7,6 @@ use BookStack\Facades\Theme;
use BookStack\Theming\ThemeEvents;
use BookStack\Users\Models\Role;
use BookStack\Users\Models\User;
use GuzzleHttp\Psr7\Request;
use GuzzleHttp\Psr7\Response;
use Illuminate\Testing\TestResponse;
use Tests\Helpers\OidcJwtHelper;
@ -137,7 +136,7 @@ class OidcTest extends TestCase
$this->post('/oidc/login');
$state = session()->get('oidc_state');
$transactions = &$this->mockHttpClient([$this->getMockAuthorizationResponse([
$transactions = $this->mockHttpClient([$this->getMockAuthorizationResponse([
'email' => 'benny@example.com',
'sub' => 'benny1010101',
])]);
@ -146,9 +145,8 @@ class OidcTest extends TestCase
// App calls token endpoint to get id token
$resp = $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state);
$resp->assertRedirect('/');
$this->assertCount(1, $transactions);
/** @var Request $tokenRequest */
$tokenRequest = $transactions[0]['request'];
$this->assertEquals(1, $transactions->requestCount());
$tokenRequest = $transactions->latestRequest();
$this->assertEquals('https://oidc.local/token', (string) $tokenRequest->getUri());
$this->assertEquals('POST', $tokenRequest->getMethod());
$this->assertEquals('Basic ' . base64_encode(OidcJwtHelper::defaultClientId() . ':testpass'), $tokenRequest->getHeader('Authorization')[0]);
@ -279,7 +277,7 @@ class OidcTest extends TestCase
{
$this->withAutodiscovery();
$transactions = &$this->mockHttpClient([
$transactions = $this->mockHttpClient([
$this->getAutoDiscoveryResponse(),
$this->getJwksResponse(),
]);
@ -289,11 +287,9 @@ class OidcTest extends TestCase
$this->runLogin();
$this->assertTrue(auth()->check());
/** @var Request $discoverRequest */
$discoverRequest = $transactions[0]['request'];
/** @var Request $discoverRequest */
$keysRequest = $transactions[1]['request'];
$discoverRequest = $transactions->requestAt(0);
$keysRequest = $transactions->requestAt(1);
$this->assertEquals('GET', $keysRequest->getMethod());
$this->assertEquals('GET', $discoverRequest->getMethod());
$this->assertEquals(OidcJwtHelper::defaultIssuer() . '/.well-known/openid-configuration', $discoverRequest->getUri());
@ -316,7 +312,7 @@ class OidcTest extends TestCase
{
$this->withAutodiscovery();
$transactions = &$this->mockHttpClient([
$transactions = $this->mockHttpClient([
$this->getAutoDiscoveryResponse(),
$this->getJwksResponse(),
$this->getAutoDiscoveryResponse([
@ -327,15 +323,15 @@ class OidcTest extends TestCase
// Initial run
$this->post('/oidc/login');
$this->assertCount(2, $transactions);
$this->assertEquals(2, $transactions->requestCount());
// Second run, hits cache
$this->post('/oidc/login');
$this->assertCount(2, $transactions);
$this->assertEquals(2, $transactions->requestCount());
// Third run, different issuer, new cache key
config()->set(['oidc.issuer' => 'https://auto.example.com']);
$this->post('/oidc/login');
$this->assertCount(4, $transactions);
$this->assertEquals(4, $transactions->requestCount());
}
public function test_auth_login_with_autodiscovery_with_keys_that_do_not_have_alg_property()

24
tests/TestCase.php
View File

@ -3,13 +3,11 @@
namespace Tests;
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 GuzzleHttp\Client;
use GuzzleHttp\Handler\MockHandler;
use GuzzleHttp\HandlerStack;
use GuzzleHttp\Middleware;
use Illuminate\Contracts\Console\Kernel;
use Illuminate\Foundation\Testing\DatabaseTransactions;
use Illuminate\Foundation\Testing\TestCase as BaseTestCase;
@ -21,7 +19,6 @@ use Illuminate\Testing\Assert as PHPUnit;
use Mockery;
use Monolog\Handler\TestHandler;
use Monolog\Logger;
use Psr\Http\Client\ClientInterface;
use Ssddanbrown\AssertHtml\TestsHtml;
use Tests\Helpers\EntityProvider;
use Tests\Helpers\FileProvider;
@ -115,6 +112,7 @@ abstract class TestCase extends BaseTestCase
*/
protected function mockHttpFetch($returnData, int $times = 1)
{
// TODO - Remove
$mockHttp = Mockery::mock(HttpFetcher::class);
$this->app[HttpFetcher::class] = $mockHttp;
$mockHttp->shouldReceive('fetch')
@ -123,21 +121,11 @@ abstract class TestCase extends BaseTestCase
}
/**
* Mock the http client used in BookStack.
* Returns a reference to the container which holds all history of http transactions.
*
* @link https://docs.guzzlephp.org/en/stable/testing.html#history-middleware
* Mock the http client used in BookStack http calls.
*/
protected function &mockHttpClient(array $responses = []): array
protected function mockHttpClient(array $responses = []): HttpClientHistory
{
$container = [];
$history = Middleware::history($container);
$mock = new MockHandler($responses);
$handlerStack = new HandlerStack($mock);
$handlerStack->push($history);
$this->app[ClientInterface::class] = new Client(['handler' => $handlerStack]);
return $container;
return $this->app->make(HttpRequestService::class)->mockClient($responses);
}
/**

14
tests/ThemeTest.php
View File

@ -12,13 +12,10 @@ use BookStack\Facades\Theme;
use BookStack\Theming\ThemeEvents;
use BookStack\Users\Models\User;
use Illuminate\Console\Command;
use Illuminate\Http\Client\Request as HttpClientRequest;
use Illuminate\Http\Request;
use Illuminate\Http\Response;
use Illuminate\Support\Facades\Artisan;
use Illuminate\Support\Facades\File;
use Illuminate\Support\Facades\Http;
use League\CommonMark\ConfigurableEnvironmentInterface;
use League\CommonMark\Environment\Environment;
class ThemeTest extends TestCase
@ -177,9 +174,7 @@ class ThemeTest extends TestCase
};
Theme::listen(ThemeEvents::WEBHOOK_CALL_BEFORE, $callback);
Http::fake([
'*' => Http::response('', 200),
]);
$responses = $this->mockHttpClient([new \GuzzleHttp\Psr7\Response(200, [], '')]);
$webhook = new Webhook(['name' => 'Test webhook', 'endpoint' => 'https://example.com']);
$webhook->save();
@ -193,9 +188,10 @@ class ThemeTest extends TestCase
$this->assertEquals($webhook->id, $args[1]->id);
$this->assertEquals($detail->id, $args[2]->id);
Http::assertSent(function (HttpClientRequest $request) {
return $request->isJson() && $request->data()['test'] === 'hello!';
});
$this->assertEquals(1, $responses->requestCount());
$request = $responses->latestRequest();
$reqData = json_decode($request->getBody(), true);
$this->assertEquals('hello!', $reqData['test']);
}
public function test_event_activity_logged()