From 00eedafbfdf2e60c7ffebb7e7ccf2a65ffa75b51 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 3 Jan 2022 19:42:48 +0000 Subject: [PATCH] Added timeout and debugging statuses to webhooks - Added a user-configurable timeout option to webhooks. - Added webhook fields for last-call/error datetime, in addition to last error string, which are shown on webhook edit view. Related to #3122 --- app/Actions/DispatchWebhookJob.php | 18 ++- app/Actions/Webhook.php | 12 +- app/Http/Controllers/WebhookController.php | 2 + database/factories/Actions/WebhookFactory.php | 1 + ...041_add_webhooks_timeout_error_columns.php | 38 ++++++ resources/lang/en/common.php | 1 + resources/lang/en/settings.php | 6 + resources/views/form/number.blade.php | 12 ++ resources/views/settings/roles/form.blade.php | 0 .../views/settings/webhooks/create.blade.php | 16 ++- .../views/settings/webhooks/edit.blade.php | 44 ++++++- .../settings/webhooks/parts/form.blade.php | 121 ++++++++---------- tests/Actions/WebhookCallTest.php | 14 +- 13 files changed, 205 insertions(+), 80 deletions(-) create mode 100644 database/migrations/2022_01_03_154041_add_webhooks_timeout_error_columns.php create mode 100644 resources/views/form/number.blade.php delete mode 100644 resources/views/settings/roles/form.blade.php diff --git a/app/Actions/DispatchWebhookJob.php b/app/Actions/DispatchWebhookJob.php index 57cb2feab..7cc530c10 100644 --- a/app/Actions/DispatchWebhookJob.php +++ b/app/Actions/DispatchWebhookJob.php @@ -72,21 +72,31 @@ class DispatchWebhookJob implements ShouldQueue { $themeResponse = Theme::dispatch(ThemeEvents::WEBHOOK_CALL_BEFORE, $this->event, $this->webhook, $this->detail); $webhookData = $themeResponse ?? $this->buildWebhookData(); + $lastError = null; try { $response = Http::asJson() ->withOptions(['allow_redirects' => ['strict' => true]]) - ->timeout(3) + ->timeout($this->webhook->timeout) ->post($this->webhook->endpoint, $webhookData); } catch (\Exception $exception) { - Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with error \"{$exception->getMessage()}\""); - return; + $lastError = $exception->getMessage(); + Log::error("Webhook call to endpoint {$this->webhook->endpoint} failed with error \"{$lastError}\""); } - if ($response->failed()) { + 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()}"); } + + $this->webhook->last_called_at = now(); + if ($lastError) { + $this->webhook->last_errored_at = now(); + $this->webhook->last_error = $lastError; + } + + $this->webhook->save(); } protected function buildWebhookData(): array diff --git a/app/Actions/Webhook.php b/app/Actions/Webhook.php index 2c0bd0f15..72a67ad92 100644 --- a/app/Actions/Webhook.php +++ b/app/Actions/Webhook.php @@ -3,6 +3,7 @@ namespace BookStack\Actions; use BookStack\Interfaces\Loggable; +use Carbon\Carbon; use Illuminate\Database\Eloquent\Collection; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; @@ -14,13 +15,22 @@ use Illuminate\Database\Eloquent\Relations\HasMany; * @property string $endpoint * @property Collection $trackedEvents * @property bool $active + * @property int $timeout + * @property string $last_error + * @property Carbon $last_called_at + * @property Carbon $last_errored_at */ class Webhook extends Model implements Loggable { - protected $fillable = ['name', 'endpoint']; + protected $fillable = ['name', 'endpoint', 'timeout']; use HasFactory; + protected $casts = [ + 'last_called_at' => 'datetime', + 'last_errored_at' => 'datetime', + ]; + /** * Define the tracked event relation a webhook. */ diff --git a/app/Http/Controllers/WebhookController.php b/app/Http/Controllers/WebhookController.php index eca3002c6..81e3b7792 100644 --- a/app/Http/Controllers/WebhookController.php +++ b/app/Http/Controllers/WebhookController.php @@ -46,6 +46,7 @@ class WebhookController extends Controller 'endpoint' => ['required', 'url', 'max:500'], 'events' => ['required', 'array'], 'active' => ['required'], + 'timeout' => ['required', 'integer', 'min:1', 'max:600'], ]); $webhook = new Webhook($validated); @@ -81,6 +82,7 @@ class WebhookController extends Controller 'endpoint' => ['required', 'url', 'max:500'], 'events' => ['required', 'array'], 'active' => ['required'], + 'timeout' => ['required', 'integer', 'min:1', 'max:600'], ]); /** @var Webhook $webhook */ diff --git a/database/factories/Actions/WebhookFactory.php b/database/factories/Actions/WebhookFactory.php index 205156793..662f64f8b 100644 --- a/database/factories/Actions/WebhookFactory.php +++ b/database/factories/Actions/WebhookFactory.php @@ -20,6 +20,7 @@ class WebhookFactory extends Factory 'name' => 'My webhook for ' . $this->faker->country(), 'endpoint' => $this->faker->url, 'active' => true, + 'timeout' => 3, ]; } } diff --git a/database/migrations/2022_01_03_154041_add_webhooks_timeout_error_columns.php b/database/migrations/2022_01_03_154041_add_webhooks_timeout_error_columns.php new file mode 100644 index 000000000..c7258d0f5 --- /dev/null +++ b/database/migrations/2022_01_03_154041_add_webhooks_timeout_error_columns.php @@ -0,0 +1,38 @@ +unsignedInteger('timeout')->default(3); + $table->text('last_error')->default(''); + $table->timestamp('last_called_at')->nullable(); + $table->timestamp('last_errored_at')->nullable(); + }); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::table('webhooks', function (Blueprint $table) { + $table->dropColumn('timeout'); + $table->dropColumn('last_error'); + $table->dropColumn('last_called_at'); + $table->dropColumn('last_errored_at'); + }); + } +} diff --git a/resources/lang/en/common.php b/resources/lang/en/common.php index 53db3cf40..013042134 100644 --- a/resources/lang/en/common.php +++ b/resources/lang/en/common.php @@ -74,6 +74,7 @@ return [ 'status' => 'Status', 'status_active' => 'Active', 'status_inactive' => 'Inactive', + 'never' => 'Never', // Header 'header_menu_expand' => 'Expand Header Menu', diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php index 08d4e7294..65e2e5264 100755 --- a/resources/lang/en/settings.php +++ b/resources/lang/en/settings.php @@ -246,6 +246,7 @@ return [ 'webhooks_events_warning' => 'Keep in mind that these events will be triggered for all selected events, even if custom permissions are applied. Ensure that use of this webhook won\'t expose confidential content.', 'webhooks_events_all' => 'All system events', 'webhooks_name' => 'Webhook Name', + 'webhooks_timeout' => 'Webhook Request Timeout (Seconds)', 'webhooks_endpoint' => 'Webhook Endpoint', 'webhooks_active' => 'Webhook Active', 'webhook_events_table_header' => 'Events', @@ -254,6 +255,11 @@ return [ 'webhooks_delete_confirm' => 'Are you sure you want to delete this webhook?', 'webhooks_format_example' => 'Webhook Format Example', 'webhooks_format_example_desc' => 'Webhook data is sent as a POST request to the configured endpoint as JSON following the format below. The "related_item" and "url" properties are optional and will depend on the type of event triggered.', + 'webhooks_status' => 'Webhook Status', + 'webhooks_last_called' => 'Last Called:', + 'webhooks_last_errored' => 'Last Errored:', + 'webhooks_last_error_message' => 'Last Error Message:', + //! If editing translations files directly please ignore this in all //! languages apart from en. Content will be auto-copied from en. diff --git a/resources/views/form/number.blade.php b/resources/views/form/number.blade.php new file mode 100644 index 000000000..a37cd3694 --- /dev/null +++ b/resources/views/form/number.blade.php @@ -0,0 +1,12 @@ +has($name)) class="text-neg" @endif + @if(isset($placeholder)) placeholder="{{$placeholder}}" @endif + @if($autofocus ?? false) autofocus @endif + @if($disabled ?? false) disabled="disabled" @endif + @if($readonly ?? false) readonly="readonly" @endif + @if($min ?? false) min="{{ $min }}" @endif + @if($max ?? false) max="{{ $max }}" @endif + @if(isset($model) || old($name)) value="{{ old($name) ? old($name) : $model->$name}}" @endif> +@if($errors->has($name)) +
{{ $errors->first($name) }}
+@endif diff --git a/resources/views/settings/roles/form.blade.php b/resources/views/settings/roles/form.blade.php deleted file mode 100644 index e69de29bb..000000000 diff --git a/resources/views/settings/webhooks/create.blade.php b/resources/views/settings/webhooks/create.blade.php index 4f20dd077..f7a99c725 100644 --- a/resources/views/settings/webhooks/create.blade.php +++ b/resources/views/settings/webhooks/create.blade.php @@ -8,9 +8,19 @@ @include('settings.parts.navbar', ['selected' => 'webhooks']) -
- @include('settings.webhooks.parts.form', ['title' => trans('settings.webhooks_create')]) -
+
+

{{ trans('settings.webhooks_create') }}

+ +
+ {!! csrf_field() !!} + @include('settings.webhooks.parts.form', ['title' => trans('settings.webhooks_create')]) + +
+ {{ trans('common.cancel') }} + +
+
+
@include('settings.webhooks.parts.format-example') diff --git a/resources/views/settings/webhooks/edit.blade.php b/resources/views/settings/webhooks/edit.blade.php index 3b297eb7b..27f3070ca 100644 --- a/resources/views/settings/webhooks/edit.blade.php +++ b/resources/views/settings/webhooks/edit.blade.php @@ -7,10 +7,46 @@ @include('settings.parts.navbar', ['selected' => 'webhooks']) -
- {!! method_field('PUT') !!} - @include('settings.webhooks.parts.form', ['model' => $webhook, 'title' => trans('settings.webhooks_edit')]) -
+
+

{{ trans('settings.webhooks_edit') }}

+ + +
+
+
+ +

+ {{ trans('settings.webhooks_last_called') }} {{ $webhook->last_called_at ? $webhook->last_called_at->diffForHumans() : trans('common.never') }} +
+ {{ trans('settings.webhooks_last_errored') }} {{ $webhook->last_errored_at ? $webhook->last_errored_at->diffForHumans() : trans('common.never') }} +

+
+
+
+ @if($webhook->last_error) + {{ trans('settings.webhooks_last_error_message') }}
+ {{ $webhook->last_error }} + @endif +
+
+
+ + +
+ +
+ {!! csrf_field() !!} + {!! method_field('PUT') !!} + @include('settings.webhooks.parts.form', ['model' => $webhook, 'title' => trans('settings.webhooks_edit')]) + +
+ {{ trans('common.cancel') }} + {{ trans('settings.webhooks_delete') }} + +
+ +
+
@include('settings.webhooks.parts.format-example') diff --git a/resources/views/settings/webhooks/parts/form.blade.php b/resources/views/settings/webhooks/parts/form.blade.php index 458b6767b..c8592e266 100644 --- a/resources/views/settings/webhooks/parts/form.blade.php +++ b/resources/views/settings/webhooks/parts/form.blade.php @@ -1,75 +1,64 @@ -{!! csrf_field() !!} +
-
-

{{ $title }}

- -
- -
+
+
+ +

{{ trans('settings.webhooks_details_desc') }}

- -

{{ trans('settings.webhooks_details_desc') }}

-
- @include('form.toggle-switch', [ - 'name' => 'active', - 'value' => old('active') ?? $model->active ?? true, - 'label' => trans('settings.webhooks_active'), - ]) - @include('form.errors', ['name' => 'active']) -
-
-
-
- - @include('form.text', ['name' => 'name']) -
-
- - @include('form.text', ['name' => 'endpoint']) -
-
-
- -
- - @include('form.errors', ['name' => 'events']) - -

{{ trans('settings.webhooks_events_desc') }}

-

{{ trans('settings.webhooks_events_warning') }}

- -
- @include('form.custom-checkbox', [ - 'name' => 'events[]', - 'value' => 'all', - 'label' => trans('settings.webhooks_events_all'), - 'checked' => old('events') ? in_array('all', old('events')) : (isset($webhook) ? $webhook->tracksEvent('all') : false), + @include('form.toggle-switch', [ + 'name' => 'active', + 'value' => old('active') ?? $model->active ?? true, + 'label' => trans('settings.webhooks_active'), ]) -
- -
- -
- @foreach(\BookStack\Actions\ActivityType::all() as $activityType) -
- @include('form.custom-checkbox', [ - 'name' => 'events[]', - 'value' => $activityType, - 'label' => $activityType, - 'checked' => old('events') ? in_array($activityType, old('events')) : (isset($webhook) ? $webhook->tracksEvent($activityType) : false), - ]) -
- @endforeach + @include('form.errors', ['name' => 'active']) +
+
+
+
+ + @include('form.text', ['name' => 'name']) +
+
+ + @include('form.text', ['name' => 'endpoint']) +
+
+ + @include('form.number', ['name' => 'timeout', 'min' => 1, 'max' => 600])
-
-
- {{ trans('common.cancel') }} - @if ($webhook->id ?? false) - {{ trans('settings.webhooks_delete') }} - @endif - +
+ + @include('form.errors', ['name' => 'events']) + +

{{ trans('settings.webhooks_events_desc') }}

+

{{ trans('settings.webhooks_events_warning') }}

+ +
+ @include('form.custom-checkbox', [ + 'name' => 'events[]', + 'value' => 'all', + 'label' => trans('settings.webhooks_events_all'), + 'checked' => old('events') ? in_array('all', old('events')) : (isset($webhook) ? $webhook->tracksEvent('all') : false), + ]) +
+ +
+ +
+ @foreach(\BookStack\Actions\ActivityType::all() as $activityType) +
+ @include('form.custom-checkbox', [ + 'name' => 'events[]', + 'value' => $activityType, + 'label' => $activityType, + 'checked' => old('events') ? in_array($activityType, old('events')) : (isset($webhook) ? $webhook->tracksEvent($activityType) : false), + ]) +
+ @endforeach +
-
+
\ No newline at end of file diff --git a/tests/Actions/WebhookCallTest.php b/tests/Actions/WebhookCallTest.php index 920bc2864..d9f9ddad5 100644 --- a/tests/Actions/WebhookCallTest.php +++ b/tests/Actions/WebhookCallTest.php @@ -53,11 +53,16 @@ class WebhookCallTest extends TestCase Http::fake([ '*' => Http::response('', 500), ]); - $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.com'], ['all']); + $webhook = $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.com'], ['all']); + $this->assertNull($webhook->last_errored_at); $this->runEvent(ActivityType::ROLE_CREATE); $this->assertTrue($logger->hasError('Webhook call to endpoint https://wh.example.com failed with status 500')); + + $webhook->refresh(); + $this->assertEquals('Response status from endpoint was 500', $webhook->last_error); + $this->assertNotNull($webhook->last_errored_at); } public function test_webhook_call_exception_is_caught_and_logged() @@ -65,11 +70,16 @@ class WebhookCallTest extends TestCase Http::shouldReceive('asJson')->andThrow(new \Exception('Failed to perform request')); $logger = $this->withTestLogger(); - $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.com'], ['all']); + $webhook = $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.com'], ['all']); + $this->assertNull($webhook->last_errored_at); $this->runEvent(ActivityType::ROLE_CREATE); $this->assertTrue($logger->hasError('Webhook call to endpoint https://wh.example.com failed with error "Failed to perform request"')); + + $webhook->refresh(); + $this->assertEquals('Failed to perform request', $webhook->last_error); + $this->assertNotNull($webhook->last_errored_at); } public function test_webhook_call_data_format()