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
2024-10-01 01:36:00 -04:00 · 2022-01-03 19:42:48 +00:00 · 2022-01-03 19:42:48 +00:00 · 00eedafbfd
commit 00eedafbfd
parent 6e18620a0a
13 changed files with 205 additions and 80 deletions
--- 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
--- 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.
     */
--- 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 */
--- 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,
        ];
    }
 }
--- 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
@ -0,0 +1,38 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\Schema;
+
+class AddWebhooksTimeoutErrorColumns extends Migration
+{
+    /**
+     * Run the migrations.
+     *
+     * @return void
+     */
+    public function up()
+    {
+        Schema::table('webhooks', function (Blueprint $table) {
+            $table->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');
+        });
+    }
+}
--- 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',
--- 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.
--- a/resources/views/form/number.blade.php
+++ b/resources/views/form/number.blade.php
@ -0,0 +1,12 @@
+<input type="number" id="{{ $name }}" name="{{ $name }}"
+       @if($errors->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))
+    <div class="text-neg text-small">{{ $errors->first($name) }}</div>
+@endif
--- a/resources/views/settings/roles/form.blade.php
+++ b/resources/views/settings/roles/form.blade.php
--- 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'])
        </div>

-        <form action="{{ url("/settings/webhooks/create") }}" method="POST">
-            @include('settings.webhooks.parts.form', ['title' => trans('settings.webhooks_create')])
-        </form>
+        <div class="card content-wrap auto-height">
+            <h1 class="list-heading">{{ trans('settings.webhooks_create') }}</h1>
+
+            <form action="{{ url("/settings/webhooks/create") }}" method="POST">
+                {!! csrf_field() !!}
+                @include('settings.webhooks.parts.form', ['title' => trans('settings.webhooks_create')])
+
+                <div class="form-group text-right">
+                    <a href="{{ url("/settings/webhooks") }}" class="button outline">{{ trans('common.cancel') }}</a>
+                    <button type="submit" class="button">{{ trans('settings.webhooks_save') }}</button>
+                </div>
+            </form>
+        </div>

        @include('settings.webhooks.parts.format-example')
    </div>
--- 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'])
        </div>

-        <form action="{{ $webhook->getUrl() }}" method="POST">
-            {!! method_field('PUT') !!}
-            @include('settings.webhooks.parts.form', ['model' => $webhook, 'title' => trans('settings.webhooks_edit')])
-        </form>
+        <div class="card content-wrap auto-height">
+            <h1 class="list-heading">{{ trans('settings.webhooks_edit') }}</h1>
+
+
+            <div class="setting-list">
+            <div class="grid half">
+                <div>
+                    <label class="setting-list-label">{{ trans('settings.webhooks_status') }}</label>
+                    <p class="mb-none">
+                        {{ trans('settings.webhooks_last_called') }} {{ $webhook->last_called_at ? $webhook->last_called_at->diffForHumans() : trans('common.never') }}
+                        <br>
+                        {{ trans('settings.webhooks_last_errored') }} {{ $webhook->last_errored_at ? $webhook->last_errored_at->diffForHumans() : trans('common.never') }}
+                    </p>
+                </div>
+                <div class="text-muted">
+                    <br>
+                    @if($webhook->last_error)
+                        {{ trans('settings.webhooks_last_error_message') }} <br>
+                        <span class="text-warn text-small">{{ $webhook->last_error }}</span>
+                    @endif
+                </div>
+            </div>
+            </div>
+
+
+            <hr>
+
+            <form action="{{ $webhook->getUrl() }}" method="POST">
+                {!! csrf_field() !!}
+                {!! method_field('PUT') !!}
+                @include('settings.webhooks.parts.form', ['model' => $webhook, 'title' => trans('settings.webhooks_edit')])
+
+                <div class="form-group text-right">
+                    <a href="{{ url("/settings/webhooks") }}" class="button outline">{{ trans('common.cancel') }}</a>
+                    <a href="{{ $webhook->getUrl('/delete') }}" class="button outline">{{ trans('settings.webhooks_delete') }}</a>
+                    <button type="submit" class="button">{{ trans('settings.webhooks_save') }}</button>
+                </div>
+
+            </form>
+        </div>

        @include('settings.webhooks.parts.format-example')
    </div>
--- a/resources/views/settings/webhooks/parts/form.blade.php
+++ b/resources/views/settings/webhooks/parts/form.blade.php
@ -1,75 +1,64 @@
-{!! csrf_field() !!}
+<div class="setting-list">

-<div class="card content-wrap auto-height">
-    <h1 class="list-heading">{{ $title }}</h1>
-
-    <div class="setting-list">
-
-        <div class="grid half">
+    <div class="grid half">
+        <div>
+            <label class="setting-list-label">{{ trans('settings.webhooks_details') }}</label>
+            <p class="small">{{ trans('settings.webhooks_details_desc') }}</p>
            <div>
-                <label class="setting-list-label">{{ trans('settings.webhooks_details') }}</label>
-                <p class="small">{{ trans('settings.webhooks_details_desc') }}</p>
-                <div>
-                    @include('form.toggle-switch', [
-                        'name' => 'active',
-                        'value' => old('active') ?? $model->active ?? true,
-                        'label' => trans('settings.webhooks_active'),
-                    ])
-                    @include('form.errors', ['name' => 'active'])
-                </div>
-            </div>
-            <div>
-                <div class="form-group">
-                    <label for="name">{{ trans('settings.webhooks_name') }}</label>
-                    @include('form.text', ['name' => 'name'])
-                </div>
-                <div class="form-group">
-                    <label for="endpoint">{{ trans('settings.webhooks_endpoint') }}</label>
-                    @include('form.text', ['name' => 'endpoint'])
-                </div>
-            </div>
-        </div>
-
-        <div component="webhook-events">
-            <label class="setting-list-label">{{ trans('settings.webhooks_events') }}</label>
-            @include('form.errors', ['name' => 'events'])
-
-            <p class="small">{{ trans('settings.webhooks_events_desc') }}</p>
-            <p class="text-warn small">{{ trans('settings.webhooks_events_warning') }}</p>
-
-            <div class="toggle-switch-list">
-                @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'),
                ])
-            </div>
-
-            <hr class="my-s">
-
-            <div class="dual-column-content toggle-switch-list">
-                @foreach(\BookStack\Actions\ActivityType::all() as $activityType)
-                    <div>
-                        @include('form.custom-checkbox', [
-                           'name' => 'events[]',
-                           'value' => $activityType,
-                           'label' => $activityType,
-                           'checked' => old('events') ? in_array($activityType, old('events')) : (isset($webhook) ? $webhook->tracksEvent($activityType) : false),
-                       ])
-                    </div>
-                @endforeach
+                @include('form.errors', ['name' => 'active'])
+            </div>
+        </div>
+        <div>
+            <div class="form-group">
+                <label for="name">{{ trans('settings.webhooks_name') }}</label>
+                @include('form.text', ['name' => 'name'])
+            </div>
+            <div class="form-group">
+                <label for="endpoint">{{ trans('settings.webhooks_endpoint') }}</label>
+                @include('form.text', ['name' => 'endpoint'])
+            </div>
+            <div class="form-group">
+                <label for="endpoint">{{ trans('settings.webhooks_timeout') }}</label>
+                @include('form.number', ['name' => 'timeout', 'min' => 1, 'max' => 600])
            </div>
        </div>
-
    </div>

-    <div class="form-group text-right">
-        <a href="{{ url("/settings/webhooks") }}" class="button outline">{{ trans('common.cancel') }}</a>
-        @if ($webhook->id ?? false)
-            <a href="{{ $webhook->getUrl('/delete') }}" class="button outline">{{ trans('settings.webhooks_delete') }}</a>
-        @endif
-        <button type="submit" class="button">{{ trans('settings.webhooks_save') }}</button>
+    <div component="webhook-events">
+        <label class="setting-list-label">{{ trans('settings.webhooks_events') }}</label>
+        @include('form.errors', ['name' => 'events'])
+
+        <p class="small">{{ trans('settings.webhooks_events_desc') }}</p>
+        <p class="text-warn small">{{ trans('settings.webhooks_events_warning') }}</p>
+
+        <div class="toggle-switch-list">
+            @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),
+            ])
+        </div>
+
+        <hr class="my-s">
+
+        <div class="dual-column-content toggle-switch-list">
+            @foreach(\BookStack\Actions\ActivityType::all() as $activityType)
+                <div>
+                    @include('form.custom-checkbox', [
+                       'name' => 'events[]',
+                       'value' => $activityType,
+                       'label' => $activityType,
+                       'checked' => old('events') ? in_array($activityType, old('events')) : (isset($webhook) ? $webhook->tracksEvent($activityType) : false),
+                   ])
+                </div>
+            @endforeach
+        </div>
    </div>

 </div>
--- 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()