Skip to content

Webhooks for some delete operations are not sent. #4373

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
toras9000 opened this issue Jul 9, 2023 · 2 comments
Closed

Webhooks for some delete operations are not sent. #4373

toras9000 opened this issue Jul 9, 2023 · 2 comments

Comments

@toras9000
Copy link

Describe the Bug

When performing entity deletion in the browser, webhooks are not sent for some operations.
Specifically, revision_delete, user_delete, api_token_delete, and webhook_delete.

When these operations are performed with webhooks set for the operations, HTTP status 404 seems to be returned.
If no webhook is set, the response is normal.

Steps to Reproduce

  1. Add a Webhook.
    • It does not seem to matter whether the endpoint address is valid or invalid.
    • Turn on 'All system events'.
  2. Perform operations corresponding to the problematic webhook events described above.
    • Webhook does not reach the endpoint.

Expected Behaviour

Webhook is sent.

Screenshots or Additional Context

No response

Browser Details

No response

Exact BookStack Version

v23.06.1

PHP Version

No response

Hosting Environment

Docker image, lscr.io/linuxserver/bookstack:version-v23.06.1

@toras9000
Copy link
Author

This may not be necessary, but I have done some research and will describe it.

I think the problem may be in the DispatchWebhookJob dispatch.
Does the deleted $detail cause problems with serialization when queuing?

In the case of page revisions, it would appear that simply changing the order of the following would work. (at least on the surface.)
(PageRevisionController.php)

$revision->delete();
Activity::add(ActivityType::REVISION_DELETE, $revision);

ssddanbrown added a commit that referenced this issue Jul 12, 2023
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
@ssddanbrown
Copy link
Member

Thanks for reporting @toras9000, Can confirm, did indeed seem due to Laravel's smart serialiszation and handling of models when in the queue, where it'd attempt to re-fetch models from the database on queue job run.

I've instead changed how the job is handled, so now the webhook data is formatted on job creation, and that data is stored with the queue job instead of the model itself.

I've always put the activity logs after the actual delete action, just to ensure that activities are not logged in the event the deletion fails, so I didn't want to switch that up.

Patch in a831501, to be part of the next patch release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants