From a83150131a900a4ec8175e87da323eda254fab86 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 12 Jul 2023 16:16:12 +0100 Subject: [PATCH] Webhooks: Fixed failing delete-based events Due to queue serialization. Added a test to check a couple of delete events. Added ApiTokenFactory to support. Also made a couple of typing/doc updates while there. Related to #4373 --- app/Activity/DispatchWebhookJob.php | 18 +++++---------- app/Activity/Tools/WebhookFormatter.php | 8 ++----- app/Api/ApiToken.php | 3 +++ app/Theming/ThemeEvents.php | 9 ++++---- database/factories/Api/ApiTokenFactory.php | 27 ++++++++++++++++++++++ tests/Actions/WebhookCallTest.php | 22 +++++++++++++++++- 6 files changed, 64 insertions(+), 23 deletions(-) create mode 100644 database/factories/Api/ApiTokenFactory.php diff --git a/app/Activity/DispatchWebhookJob.php b/app/Activity/DispatchWebhookJob.php index 5e6380f43..f2330c4fa 100644 --- a/app/Activity/DispatchWebhookJob.php +++ b/app/Activity/DispatchWebhookJob.php @@ -24,27 +24,23 @@ class DispatchWebhookJob implements ShouldQueue use SerializesModels; protected Webhook $webhook; - protected string $event; protected User $initiator; protected int $initiatedTime; - - /** - * @var string|Loggable - */ - protected $detail; + protected array $webhookData; /** * Create a new job instance. * * @return void */ - public function __construct(Webhook $webhook, string $event, $detail) + public function __construct(Webhook $webhook, string $event, Loggable|string $detail) { $this->webhook = $webhook; - $this->event = $event; - $this->detail = $detail; $this->initiator = user(); $this->initiatedTime = time(); + + $themeResponse = Theme::dispatch(ThemeEvents::WEBHOOK_CALL_BEFORE, $event, $this->webhook, $detail, $this->initiator, $this->initiatedTime); + $this->webhookData = $themeResponse ?? WebhookFormatter::getDefault($event, $this->webhook, $detail, $this->initiator, $this->initiatedTime)->format(); } /** @@ -54,15 +50,13 @@ class DispatchWebhookJob implements ShouldQueue */ public function handle() { - $themeResponse = Theme::dispatch(ThemeEvents::WEBHOOK_CALL_BEFORE, $this->event, $this->webhook, $this->detail, $this->initiator, $this->initiatedTime); - $webhookData = $themeResponse ?? WebhookFormatter::getDefault($this->event, $this->webhook, $this->detail, $this->initiator, $this->initiatedTime)->format(); $lastError = null; try { $response = Http::asJson() ->withOptions(['allow_redirects' => ['strict' => true]]) ->timeout($this->webhook->timeout) - ->post($this->webhook->endpoint, $webhookData); + ->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}\""); diff --git a/app/Activity/Tools/WebhookFormatter.php b/app/Activity/Tools/WebhookFormatter.php index 423713978..6ccb084b8 100644 --- a/app/Activity/Tools/WebhookFormatter.php +++ b/app/Activity/Tools/WebhookFormatter.php @@ -17,18 +17,14 @@ class WebhookFormatter protected string $event; protected User $initiator; protected int $initiatedTime; - - /** - * @var string|Loggable - */ - protected $detail; + protected string|Loggable $detail; /** * @var array{condition: callable(string, Model):bool, format: callable(Model):void}[] */ protected $modelFormatters = []; - public function __construct(string $event, Webhook $webhook, $detail, User $initiator, int $initiatedTime) + public function __construct(string $event, Webhook $webhook, string|Loggable $detail, User $initiator, int $initiatedTime) { $this->webhook = $webhook; $this->event = $event; diff --git a/app/Api/ApiToken.php b/app/Api/ApiToken.php index b462eaae9..5c2d591e4 100644 --- a/app/Api/ApiToken.php +++ b/app/Api/ApiToken.php @@ -4,6 +4,7 @@ namespace BookStack\Api; use BookStack\Activity\Models\Loggable; use BookStack\Users\Models\User; +use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Support\Carbon; @@ -20,6 +21,8 @@ use Illuminate\Support\Carbon; */ class ApiToken extends Model implements Loggable { + use HasFactory; + protected $fillable = ['name', 'expires_at']; protected $casts = [ 'expires_at' => 'date:Y-m-d', diff --git a/app/Theming/ThemeEvents.php b/app/Theming/ThemeEvents.php index 994c3ec0d..4b56b2f56 100644 --- a/app/Theming/ThemeEvents.php +++ b/app/Theming/ThemeEvents.php @@ -132,11 +132,12 @@ class ThemeEvents * If the listener returns a non-null value, that will be used as the POST data instead * of the system default. * - * @param string $event - * @param \BookStack\Activity\Models\Webhook $webhook + * @param string $event + * @param \BookStack\Activity\Models\Webhook $webhook * @param string|\BookStack\Activity\Models\Loggable $detail - * @param \BookStack\Users\Models\User $initiator - * @param int $initiatedTime + * @param \BookStack\Users\Models\User $initiator + * @param int $initiatedTime + * @returns array|null */ const WEBHOOK_CALL_BEFORE = 'webhook_call_before'; } diff --git a/database/factories/Api/ApiTokenFactory.php b/database/factories/Api/ApiTokenFactory.php new file mode 100644 index 000000000..adf2fff4a --- /dev/null +++ b/database/factories/Api/ApiTokenFactory.php @@ -0,0 +1,27 @@ + Str::random(10), + 'secret' => Str::random(12), + 'name' => $this->faker->name(), + 'expires_at' => Carbon::now()->addYear(), + 'created_at' => Carbon::now(), + 'updated_at' => Carbon::now(), + 'user_id' => User::factory(), + ]; + } +} diff --git a/tests/Actions/WebhookCallTest.php b/tests/Actions/WebhookCallTest.php index f2def087b..fc49a524e 100644 --- a/tests/Actions/WebhookCallTest.php +++ b/tests/Actions/WebhookCallTest.php @@ -6,6 +6,8 @@ use BookStack\Activity\ActivityType; 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 Illuminate\Support\Facades\Bus; @@ -46,6 +48,24 @@ class WebhookCallTest extends TestCase Bus::assertNotDispatched(DispatchWebhookJob::class); } + public function test_webhook_runs_for_delete_actions() + { + $this->newWebhook(['active' => true, 'endpoint' => 'https://wh.example.com'], ['all']); + Http::fake([ + '*' => Http::response('', 500), + ]); + + $user = $this->users->newUser(); + $resp = $this->asAdmin()->delete($user->getEditUrl()); + $resp->assertRedirect('/settings/users'); + + /** @var ApiToken $apiToken */ + $editor = $this->users->editor(); + $apiToken = ApiToken::factory()->create(['user_id' => $editor]); + $resp = $this->delete($editor->getEditUrl('/api-tokens/' . $apiToken->id)); + $resp->assertRedirect($editor->getEditUrl('#api_tokens')); + } + public function test_failed_webhook_call_logs_error() { $logger = $this->withTestLogger(); @@ -120,7 +140,7 @@ class WebhookCallTest extends TestCase $activityLogger->add($event, $detail); } - protected function newWebhook(array $attrs = [], array $events = ['all']): Webhook + protected function newWebhook(array $attrs, array $events): Webhook { /** @var Webhook $webhook */ $webhook = Webhook::factory()->create($attrs);