Add support for webhook-based notifications #1181

rounakdatta · 2022-06-02T13:52:18Z

rounakdatta commented

2022-06-02 13:52:18 +00:00

First-time contributor

Description

This pull request introduces webhook as a type of notification. The enablement of the webhook feature and the endpoint URL can be controlled via config.

Checklist

I added or improved tests
I added or improved docs for my feature
- Swagger (including mage do-the-swag)
- Error codes
- New config options (including adding them to config.yml.saml and running mage generate-docs)

# Description This pull request introduces webhook as a type of notification. The enablement of the webhook feature and the endpoint URL can be controlled via config. # Checklist * [ ] I added or improved tests * [ ] I added or improved docs for my feature * [ ] Swagger (including `mage do-the-swag`) * [ ] Error codes * [ ] New config options (including adding them to `config.yml.saml` and running `mage generate-docs`)

rounakdatta added 2 commits 2022-06-02 13:52:20 +00:00

6ce1a84bb0

Introduce WebhookNotification

continuous-integration/drone/pr Build is failing Details

d1a301fb3c

Add TODO comment for http retries

rounakdatta changed title from ~~WIP: webhook-notification~~ to WIP: Add support for webhook-based notifications

2022-06-02 13:52:58 +00:00

rounakdatta closed this pull request

2022-06-02 17:42:17 +00:00

rounakdatta reopened this pull request

2022-06-02 17:42:23 +00:00

konrad commented

2022-06-03 06:58:22 +00:00

Nice!

I know this is still wip, but here's a few notes of mine:

Outgoing webhooks should have a signature for receivers to verify the request came from vikunja and not someone else - see https://webhooks.dev/docs/auth/
It makes sense how you implemented it as a notification channel. In the future I'd love to have webhooks configurable per user/list/namespace, more like how GitHub does it. But that could go in a different pr after this one got merged.

Nice! I know this is still wip, but here's a few notes of mine: * Outgoing webhooks should have a signature for receivers to verify the request came from vikunja and not someone else - see https://webhooks.dev/docs/auth/ * It makes sense how you implemented it as a notification channel. In the future I'd love to have webhooks configurable per user/list/namespace, more like how GitHub does it. But that could go in a different pr after this one got merged.

👍 1

rounakdatta added 1 commit 2022-06-03 14:01:20 +00:00

continuous-integration/drone/pr Build is failing Details

c9e67570ea

Resolve lint errors

rounakdatta commented

2022-06-03 14:06:08 +00:00

First-time contributor

Thanks @konrad ,

I'm looking into the webhook signature, and will integrate it soon.
I think this is an important callout to have the webhooks configurable at user level. By implementing it now, we can avoid confusion of having a env var config now and a DB-level config later. Looking into it as well 👍

Thanks @konrad , - I'm looking into the webhook signature, and will integrate it soon. - I think this is an important callout to have the webhooks configurable at user level. By implementing it now, we can avoid confusion of having a env var config now and a DB-level config later. Looking into it as well 👍

rounakdatta added 1 commit 2022-06-03 15:21:33 +00:00

continuous-integration/drone/pr Build is failing Details

1745dbb85d

Pass HMAC in header for integrity

rounakdatta commented

2022-06-03 15:25:34 +00:00

First-time contributor

Well, keeping the endpoint URL user/list/namespace specific is turning out to be non-trivial 😅, especially since it will require migrations on multiple tables including user. As you suggested, we should probably do it in a follow-up PR.

Well, keeping the endpoint URL user/list/namespace specific is turning out to be non-trivial 😅, especially since it will require migrations on multiple tables including `user`. As you suggested, we should probably do it in a follow-up PR.

👍 1

rounakdatta changed title from ~~WIP: Add support for webhook-based notifications~~ to Add support for webhook-based notifications

2022-06-04 18:04:35 +00:00

konrad referenced this pull request

2022-06-07 18:23:29 +00:00

Event-based webhook #1183

konrad requested changes 2022-06-07 20:01:56 +00:00

konrad left a comment

Looks pretty good already!

Please fix the lint errors and add a test for the feature.

Looks pretty good already! Please fix the lint errors and add a test for the feature.

pkg/notifications/helpers.go

						
				@ -0,0 +7,4 @@

				)

				// DoPost accepts a stringified body makes a post request

				func DoPost(url string, body string, headers map[string]string) (resp *http.Response, err error) {

konrad commented

2022-06-07 19:54:59 +00:00

I wonder if this could be combined with the doPost helper from the migratiors?

I wonder if this could be combined with the [`doPost` helper](https://kolaente.dev/vikunja/api/src/branch/main/pkg/modules/migration/helpers.go#L58) from the migratiors?

pkg/notifications/notification.go

						
				@ -59,2 +60,4 @@

					}

					if config.WebhookNotificationsEnabled.GetBool() {

						err = notifyWebhook(notifiable, notification)

konrad commented

2022-06-07 19:56:54 +00:00

Even though notifications are almost always sent as a result of some async event, I think this should be done async explicitely. I'd publish a WebhookEvent or similar and then notify the url in the listener to that event.
The check whether webhook notifications are enabled or not could then happen while registering the listener for that event (= only register the listener if webhook notifications are enabled).

Even though notifications are almost always sent as a result of some async event, I think this should be done async explicitely. I'd publish a `WebhookEvent` or similar and then notify the url in the listener to that event. The check whether webhook notifications are enabled or not could then happen while registering the listener for that event (= only register the listener if webhook notifications are enabled).

pkg/notifications/webhook.go

						
				@ -0,0 +25,4 @@

				)

				const (

					webhookNotificationSignatureHeader   = "x-vikunja-signature"

konrad commented

2022-06-07 19:59:16 +00:00

Please use X-Vikunja-Signature (capitalized)

Please use `X-Vikunja-Signature` (capitalized)

pkg/notifications/webhook.go

						
				@ -0,0 +50,4 @@

						return err

					}

					defer resp.Body.Close()

					// TODO: Have response-code based checks

konrad commented

2022-06-07 19:58:44 +00:00

What should those checks look like?

konrad commented

2022-06-07 20:02:56 +00:00

Well, keeping the endpoint URL user/list/namespace specific is turning out to be non-trivial 😅, especially since it will require migrations on multiple tables including user. As you suggested, we should probably do it in a follow-up PR.

Follow-Up PR is fine by me. Keeps this one small and easy to review :)

I'm curious, why would that require a migration on the user table?

> Well, keeping the endpoint URL user/list/namespace specific is turning out to be non-trivial 😅, especially since it will require migrations on multiple tables including user. As you suggested, we should probably do it in a follow-up PR. Follow-Up PR is fine by me. Keeps this one small and easy to review :) I'm curious, why would that require a migration on the user table?

k3idii referenced this pull request

2022-06-08 05:05:18 +00:00

Event-based webhook #1183

konrad commented

2024-01-16 12:16:42 +00:00

Closing this due to inactivity. Please ping if you want to pick it up again.

konrad closed this pull request

2024-01-16 12:16:42 +00:00

konrad referenced this issue from a commit

2024-02-07 14:00:30 +00:00

feat: save and restore the user language on the server (#1181)

continuous-integration/drone/pr Build is failing