Event-based webhook #1183

Closed
k3idii wants to merge 10 commits from k3idii/api:webhook-events into main
First-time contributor

Description

Added webhook support (event based).
Simple HTTP POST of event data to specific endpoint

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 Added webhook support (event based). Simple HTTP POST of event data to specific endpoint # Checklist * [ ] I added or improved tests * [x] I added or improved docs for my feature * [ ] Swagger (including `mage do-the-swag`) * [ ] Error codes * [x] New config options (including adding them to `config.yml.saml` and running `mage generate-docs`)
k3idii added 1 commit 2022-06-07 15:19:32 +00:00
continuous-integration/drone/pr Build is failing Details
9466b1fba5
Event-based webhook
konrad reviewed 2022-06-07 18:22:38 +00:00
@ -103,6 +104,7 @@ func FullInit() {
go func() {
models.RegisterListeners()
user.RegisterListeners()
webhooks.RegisterListeners()
Owner

Using a message router middleware like in this example is probably the better aproach than registering a listener manually for each and every event.

Using a [message router middleware like in this example](https://watermill.io/docs/getting-started/#using-message-router) is probably the better aproach than registering a listener manually for each and every event.
Author
First-time contributor

I didn't try that. Mostly cause I'm not (yet) familiar w/ Watermill :)
Sound like better idea (less code)

I didn't try that. Mostly cause I'm not (yet) familiar w/ Watermill :) Sound like better idea (less code)
Owner

How can we combine this with #1181? Or would you say this is something different?

How can we combine this with #1181? Or would you say this is something different?
Author
First-time contributor

#1181 is based on "only" notification - and the PR there is doing http call in sync way (in the code handling notification).

I tried to follow the pattern of event-driven actions (how for example "user-mentions" are handled).

And I think it covers wider range of sitiations: any event can cause webhook to be executed, allowing better integration.

Additinally IMHO this is good background for future customisation: TheGreatWebhookHandler function gets event name as string - it cane be later used for filtering specific events - for example:

 events : [ 'task.*' ]
 webhookUrl: /handle-task-event 

For potential discusion on signatures/authorization/etc: I think it should be kept simple. See how discord is handling incoming webhook - the "security" is based on unique (and very long) url generated for a webhook endpoint. Adding signatures (especially for example in headers) adds potential security but in the same time adds unnecesery complication (both in sender and receipient side)

#1181 is based on "only" notification - and the PR there is doing http call in sync way (in the code handling notification). I tried to follow the pattern of event-driven actions (how for example "user-mentions" are handled). And I think it covers wider range of sitiations: any event can cause webhook to be executed, allowing better integration. Additinally IMHO this is good background for future customisation: `TheGreatWebhookHandler` function gets event name as string - it cane be later used for filtering specific events - for example: ``` events : [ 'task.*' ] webhookUrl: /handle-task-event ``` For potential discusion on signatures/authorization/etc: I think it should be kept simple. See how discord is handling incoming webhook - the "security" is based on unique (and very long) url generated for a webhook endpoint. Adding signatures (especially for example in headers) adds potential security but in the same time adds unnecesery complication (both in sender and receipient side)
Owner

I tried to follow the pattern of event-driven actions (how for example "user-mentions" are handled).

And I think it covers wider range of sitiations: any event can cause webhook to be executed, allowing better integration.

This does sound like a good approach.

Ideally this could be configured per list / namespace and would let users configure what events to subscribe to, similar to how Github does it. That could be a later PR though.

See how discord is handling incoming webhook

That's correct, but this PR is about outgoing webhooks. Signatures should be used in that case to allow the receiving party to verify the request came from Vikunja and not from someone else trying to impersonate Vikunja.

[Discord also sends signatures with every outgoing webhook](See how discord is handling incoming webhook).

> I tried to follow the pattern of event-driven actions (how for example "user-mentions" are handled). > > And I think it covers wider range of sitiations: any event can cause webhook to be executed, allowing better integration. This does sound like a good approach. Ideally this could be configured per list / namespace and would let users configure what events to subscribe to, similar to how Github does it. That could be a later PR though. > See how discord is handling incoming webhook That's correct, but this PR is about _outgoing_ webhooks. Signatures should be used in that case to allow the receiving party to verify the request came from Vikunja and not from someone else trying to impersonate Vikunja. [Discord also sends signatures with every outgoing webhook](See how discord is handling incoming webhook).
k3idii added 1 commit 2022-06-14 00:31:21 +00:00
continuous-integration/drone/pr Build is failing Details
44128ceb13
router/middlware based webhooks.
k3idii added 1 commit 2022-06-14 00:31:36 +00:00
continuous-integration/drone/pr Build is failing Details
bc53f9dcc5
Merge branch 'main' into webhook-events
Author
First-time contributor

Updated.
Using middleware/router way

  • configuration moved closer to user-defined topics:
# Webhook configuration
webhooks:
  - events : [ '*' ]
    # URL for webhook
    url: "http://127.0.0.1/test/test1/?q="
    secret : "abc123"
  - events : [ 'task.', 'foobar.' ]
    # URL for webhook
    url: "http://127.0.0.1/test/test2/"
    # no secret == no HMAC
Updated. Using middleware/router way + configuration moved closer to user-defined topics: ``` # Webhook configuration webhooks: - events : [ '*' ] # URL for webhook url: "http://127.0.0.1/test/test1/?q=" secret : "abc123" - events : [ 'task.', 'foobar.' ] # URL for webhook url: "http://127.0.0.1/test/test2/" # no secret == no HMAC ```
konrad requested changes 2022-06-16 21:01:32 +00:00
konrad left a comment
Owner

Works nicely already!

I have a few comments though. And please take a look at the CI run and fix the issues reported there.

Works nicely already! I have a few comments though. And please take a look at the CI run and fix the issues reported there.
@ -322,0 +325,4 @@
# If set to true, HTTP POST is executed
enabled: true
# URL for webhook
url: "http://127.0.0.1/test/"
Owner

Please add the secret here as well so that it shows up in the docs.

Please add the secret here as well so that it shows up in the docs.
k3idii marked this conversation as resolved
@ -161,2 +161,4 @@
MetricsUsername Key = `metrics.username`
MetricsPassword Key = `metrics.password`
WebhookConf Key = `webhooks`
Owner

Since there are multiple webhooks defined under that endpoint, I think this should be webhooks.

Since there are multiple webhooks defined under that endpoint, I think this should be `webhooks`.
k3idii marked this conversation as resolved
@ -372,0 +380,4 @@
WebhookConf.setDefault( []int{} )
// WebhookEnabled.setDefault(false)
// WebhookURL.setDefault("")
Owner

Please remove the commented lines.

Please remove the commented lines.
k3idii marked this conversation as resolved
@ -72,7 +74,10 @@ func InitEvents() (err error) {
return nil
})
whMiddleware := webhooks.GenerateMiddleware()
Owner

Why not calling webhooks.GenerateMiddleware() in the router.AddMiddleware below?

Why not calling `webhooks.GenerateMiddleware()` in the `router.AddMiddleware` below?
k3idii marked this conversation as resolved
@ -75,1 +77,4 @@
whMiddleware := webhooks.GenerateMiddleware()
router.AddMiddleware(
whMiddleware, //webhooks.WebhookMiddleware,
Owner

Please remove this commented line.

Please remove this commented line.
@ -88,1 +93,4 @@
router.AddMiddleware(
)
Owner

Please remove this - it serves no purpose.

Please remove this - it serves no purpose.
@ -0,0 +1,91 @@
package webhooks
Owner

Please add the copyright header as in the other files.

Please add the copyright header as in the other files.
k3idii marked this conversation as resolved
@ -0,0 +8,4 @@
"github.com/ThreeDotsLabs/watermill/message"
)
// type middlwareFunc func(message.HandlerFunc) message.HandlerFunc
Owner

Please remove this commented line.

Please remove this commented line.
k3idii marked this conversation as resolved
@ -0,0 +28,4 @@
func TheGreatWebhookHandler(cfg []WebhookRuntimeConfig, topic string, msg *message.Message) {
Owner

I think this function should be called something more descriptive, maybe HandleOutgoingWebhook or something like that.

I think this function should be called something more descriptive, maybe `HandleOutgoingWebhook` or something like that.
k3idii marked this conversation as resolved
@ -0,0 +34,4 @@
log.Debugf("Checking webhook config [%d]", i)
if entry.FilterFunc(topic) {
log.Debugf("Executing webhook [%d]", i)
entry.ExecuteFunc(topic, msg)
Owner

Please handle the error from that function - at least log it.

Please handle the error from that function - at least log it.
@ -0,0 +42,4 @@
/*
func OldWebhookMiddleware(h message.HandlerFunc) message.HandlerFunc {
Owner

Please remove all the commented code here.

Please remove all the commented code here.
k3idii marked this conversation as resolved
@ -0,0 +1,142 @@
Owner

Please add the copyright header as in the other files.

Please add the copyright header as in the other files.
Owner

Please add the copyright header as in the other files.

Please add the copyright header as in the other files.
k3idii marked this conversation as resolved
@ -0,0 +19,4 @@
)
const (
timeoutVal = 5 * time.Second
Owner

This should be configurable.

This should be configurable.
Author
First-time contributor

Done. in config

Done. in config
k3idii marked this conversation as resolved
@ -0,0 +21,4 @@
const (
timeoutVal = 5 * time.Second
ctHeader = "Content-Type"
ctValue = "application/json"
Owner

What does ct mean?

What does `ct` mean?
Author
First-time contributor

content-type header and value

content-type header and value
@ -0,0 +22,4 @@
timeoutVal = 5 * time.Second
ctHeader = "Content-Type"
ctValue = "application/json"
hmacHeader = "X-signature"
Owner

This should be either x-signature or X-Signature.

This should be either `x-signature` or `X-Signature`.
k3idii marked this conversation as resolved
@ -0,0 +44,4 @@
func GetWebhookFilterFunc(cfg SingleConfEntry) FilteringFunction {
Owner

Does this function need to be public? If not, please make it private.

Does this function need to be public? If not, please make it private.
k3idii marked this conversation as resolved
@ -0,0 +45,4 @@
func GetWebhookFilterFunc(cfg SingleConfEntry) FilteringFunction {
return func(topic string) (iteresting bool) {
Owner

I think you mean interesting? (typo)

I think you mean `interesting`? (typo)
k3idii marked this conversation as resolved
@ -0,0 +48,4 @@
return func(topic string) (iteresting bool) {
for _, filter := range cfg.Events {
log.Debugf("Match pattern:'%s' agains:'%s'", filter, topic)
if filter == "*" {
Owner

I think we should just use glob matching for this.

I think we should just use [glob matching](https://github.com/gobwas/glob) for this.
Author
First-time contributor

At first I was thinking this way. But after going over few use-cases I ened it like this way. My justification:

  • topic are organised in category.subcategory.etc
  • when looking for specific topic, one need will focus on specifying category or cat.subcat - not .*user.* (might be spossible but rather useless).
  • str.startswith is .. fasters :)
At first I was thinking this way. But after going over few use-cases I ened it like this way. My justification: - topic are organised in `category.subcategory.etc` - when looking for specific topic, one need will focus on specifying `category` or `cat.subcat` - not `.*user.*` (might be spossible but rather useless). - str.startswith is .. fasters :)
Owner

But glob matching would be more flexible and require less code here - you don't know how people would want to use this, maybe a webhook for *.user.* would make sense in some use-cases.

But glob matching would be more flexible and require less code here - you don't know how people would want to use this, maybe a webhook for `*.user.*` would make sense in some use-cases.
@ -0,0 +53,4 @@
return true
} else {
if strings.HasPrefix(topic, filter) {
log.Debugf(" Positive match ")
Owner

Since this is called every time a webhook is fired, it creates a lot of noise in the logs, even though the log level is set to debug.

More importantly, the log is not that useful on its own. Please either add more information like what event, what was matched, etc. or remove the log message completly.

Also, is it really nessecary to call this filter function every time a webhook is handled? Couldn't you prepare this once?

Since this is called every time a webhook is fired, it creates a lot of noise in the logs, even though the log level is set to debug. More importantly, the log is not that useful on its own. Please either add more information like what event, what was matched, etc. or remove the log message completly. Also, is it really nessecary to call this filter function every time a webhook is handled? Couldn't you prepare this once?
Author
First-time contributor

I had an idea to 'cache' interesting and non-interesing topics for each webhook after filer funciton is ran ... but did not have time to play with it for now.

I had an idea to 'cache' interesting and non-interesing topics for each webhook after filer funciton is ran ... but did not have time to play with it for now.
@ -0,0 +62,4 @@
}
}
func GetWebhookCallFunc(cfg SingleConfEntry) WebhookCallFunction {
Owner

Does this function need to be public? If not, please make it private.

Does this function need to be public? If not, please make it private.
k3idii marked this conversation as resolved
@ -0,0 +70,4 @@
log.Debugf("Webhook Call : %s (%s)", endpointURL, hmacKey)
webhookUrl := fmt.Sprintf("%s%s", endpointURL, topic)
rawData := msg.Payload
Owner

Please wrap the payload in a WebhookEvent struct or similar with the topic and payload.

Something like this:

type WebhookEvent struct {
	Topic string `json: "Topic"`
    Payload string `json: "payload"`
}

Just appending the topic to the URL is not ideal as it is rather uncommon and people won't expect it to work like that.

Please wrap the payload in a `WebhookEvent` struct or similar with the topic and payload. Something like this: ``` type WebhookEvent struct { Topic string `json: "Topic"` Payload string `json: "payload"` } ``` Just appending the topic to the URL is not ideal as it is rather uncommon and people won't expect it to work like that.
Author
First-time contributor

My idea behind that was :

  • each topic have it specific JSON payload -> each topic needs to be handled differently

  • (because above) - the routing on endpoint that would consume webhook (for example for url: http://local/handle/ ) could look like (it is common pattern)

app.route("/handle/(<topic>)")
// generic handler here 
 
 // OR
 
app.route("/handle/task.created")
// code for task.created event

  • if one want to have a webhook payload formatted in one of common format (for example 'discord incoming') - it would need to be implemented in in this codebase and would heavily limit the customization of end-product

  • I exepect that people (at least I will :) ) will implement proxy-handler that will receive webhook from vikunja, re-format it into multiple needed ways and send to (in dedicated format) to dedicated consumers (like discord, per-user notifications)

  • additinally I did not want to rebuild JSON in this code base (as event pyaload is already JSONed and don't need to be touched)

My idea behind that was : - each topic have it specific JSON payload -> each topic needs to be handled differently - (because above) - the routing on endpoint that would consume webhook (for example for url: http://local/handle/ ) could look like (it is common pattern) ``` app.route("/handle/(<topic>)") // generic handler here // OR app.route("/handle/task.created") // code for task.created event ``` - if one want to have a webhook payload formatted in one of common format (for example 'discord incoming') - it would need to be implemented in in this codebase and would heavily limit the customization of end-product - I exepect that people (at least I will :) ) will implement proxy-handler that will receive webhook from vikunja, re-format it into multiple needed ways and send to (in dedicated format) to dedicated consumers (like discord, per-user notifications) - additinally I did not want to rebuild JSON in this code base (as event pyaload is already JSONed and don't need to be touched)
Owner

The problem is you might not have complete control over the receiving end. A tool like n8n will not know what to do when to do with a request when Vikunja sends the event to <n8n's webhook url>/task.created and will probably ignore the event entierly. Other tools to that as well, it's very uncommon to have the event type included in the url.

In your handler example you could just have one handler and then a switch case statement to pass the payload to different functions based on the event type.

The problem is you might not have complete control over the receiving end. A tool like [n8n](https://n8n.io) will not know what to do when to do with a request when Vikunja sends the event to `<n8n's webhook url>/task.created` and will probably ignore the event entierly. Other tools to that as well, it's very uncommon to have the event type included in the url. In your handler example you could just have one handler and then a switch case statement to pass the payload to different functions based on the event type.
@ -0,0 +72,4 @@
rawData := msg.Payload
req, err1 := http.NewRequest( http.MethodPost, webhookUrl, bytes.NewBuffer(rawData))
Owner

Why err1? Please call this variable just err.

Why `err1`? Please call this variable just `err`.
k3idii marked this conversation as resolved
@ -0,0 +89,4 @@
req.Header.Set(hmacHeader, signature)
}
_, err2 := client.Do(req)
Owner

Why err2? Please call this variable just err.

Why `err2`? Please call this variable just `err`.
k3idii marked this conversation as resolved
@ -0,0 +91,4 @@
_, err2 := client.Do(req)
//_, err := http.Post(webhook_url, "application/json",data) "x-vikunja-signature"
Owner

Please remove the commented line of code.

Please remove the commented line of code.
k3idii marked this conversation as resolved
@ -0,0 +97,4 @@
return err2
}
log.Debugf("Webhook success : %s ", webhookUrl )
Owner

This should include the topic name as well.

This should include the topic name as well.
Author
First-time contributor

webhookUrl contains url + topic

webhookUrl contains url + topic
@ -0,0 +117,4 @@
var items []SingleConfEntry
config.WebhookConf.GetUnmarshaled(&items)
Owner

Please take a look at how we do this for openid providers: https://kolaente.dev/vikunja/api/src/branch/main/pkg/modules/auth/openid/providers.go#L48

Please take a look at how we do this for openid providers: https://kolaente.dev/vikunja/api/src/branch/main/pkg/modules/auth/openid/providers.go#L48
Author
First-time contributor

Yes, but my low golang skill was not able to reproduce.
And I found this way much simple to do and understand.

Yes, but my low golang skill was not able to reproduce. And I found this way much simple to do and understand.
@ -0,0 +121,4 @@
runtime := make([]WebhookRuntimeConfig, len(items))
fmt.Printf("RAW: %+v\n", items)
Owner

Please remove these calls to fmt or replace them with calls to appropriate logging functions.

Please remove these calls to `fmt` or replace them with calls to appropriate logging functions.
k3idii marked this conversation as resolved
k3idii added 2 commits 2022-06-19 19:51:16 +00:00
k3idii added 1 commit 2022-06-19 19:51:29 +00:00
continuous-integration/drone/pr Build is failing Details
3713bcb374
Merge branch 'main' into webhook-events
k3idii added 2 commits 2022-06-19 20:18:31 +00:00
k3idii added 2 commits 2022-06-20 06:52:45 +00:00
continuous-integration/drone/pr Build is failing Details
b018fa4f43
make lint happy2
konrad requested changes 2022-07-07 15:05:32 +00:00
@ -322,0 +326,4 @@
# URL for webhook
url: "http://127.0.0.1/test/test1/?q="
secret : "abc123"
timeout : 3
Owner

Please add comments for the secret and timeout properties. Also you set the default value here to 3 but said in a comment below the default value is 5? Which one is correct?

Also please remove the second example value and regenerate the config docs.

Please add comments for the `secret` and `timeout` properties. Also you set the default value here to `3` but said in a comment below the default value is `5`? Which one is correct? Also please remove the second example value and regenerate the config docs.
Owner

Please fix the build errors (this is the reason CI is failing):

pkg/webhooks/runtime.go:108:3: undefined: resp
pkg/webhooks/runtime.go:108:13: cannot assign *http.Response to resp in multiple assignment
pkg/webhooks/runtime.go:109:9: undefined: resp
Please fix the build errors (this is the reason CI is failing): ``` pkg/webhooks/runtime.go:108:3: undefined: resp pkg/webhooks/runtime.go:108:13: cannot assign *http.Response to resp in multiple assignment pkg/webhooks/runtime.go:109:9: undefined: resp ```
Owner

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

Closing this due to inactivity. Please ping if you want to pick it up again.
konrad closed this pull request 2024-01-16 12:17:10 +00:00
Some checks failed
continuous-integration/drone/pr Build is failing
Required
Details

Pull request closed

Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/vikunja#1183
No description provided.