Event-based webhook #1183

Closed
k3idii wants to merge 10 commits from k3idii/api:webhook-events into main
6 changed files with 254 additions and 0 deletions

View File

@ -319,3 +319,16 @@ metrics:
username:
# If set to a non-empty value the /metrics endpoint will require this as a password via basic auth in combination with the username below.
password:
# Webhook configuration
webhooks:
- events : [ '*' ]
# URL for webhook
url: "http://127.0.0.1/test/test1/?q="
secret : "abc123"
k3idii marked this conversation as resolved Outdated

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.
timeout : 3
Review

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.
- events : [ 'task.', 'foobar.' ]
# URL for webhook
url: "http://127.0.0.1/test/test2/"
# no secret == no HMAC
# no timeout == default : 5s

View File

@ -1161,3 +1161,33 @@ Full path: `metrics.password`
Environment path: `VIKUNJA_METRICS_PASSWORD`
---
## webhook
Webhook configuration
### enabled
If set to true, HTTP POST is executed
Default: `true`
Full path: `webhook.enabled`
Environment path: `VIKUNJA_WEBHOOK_ENABLED`
### url
URL for webhook
Default: `http://127.0.0.1/test/`
Full path: `webhook.url`
Environment path: `VIKUNJA_WEBHOOK_URL`

View File

@ -160,8 +160,16 @@ const (
MetricsEnabled Key = `metrics.enabled`
MetricsUsername Key = `metrics.username`
MetricsPassword Key = `metrics.password`
WebhooksConf Key = `webhooks`
k3idii marked this conversation as resolved Outdated

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`.
)
// Unmarshal object directly from config
func (k Key) GetUnmarshaled(rawVal interface{}) interface{} {
return viper.UnmarshalKey(string(k), &rawVal)
}
// GetString returns a string config value
func (k Key) GetString() string {
return viper.GetString(string(k))
@ -369,6 +377,8 @@ func InitDefaultConfig() {
KeyvalueType.setDefault("memory")
// Metrics
MetricsEnabled.setDefault(false)
WebhooksConf.setDefault([]int{})
}
k3idii marked this conversation as resolved Outdated

Please remove the commented lines.

Please remove the commented lines.
// InitConfig initializes the config, sets defaults etc.

View File

@ -28,6 +28,8 @@ import (
"github.com/ThreeDotsLabs/watermill/message"
"github.com/ThreeDotsLabs/watermill/message/router/middleware"
"github.com/ThreeDotsLabs/watermill/pubsub/gochannel"
"code.vikunja.io/api/pkg/webhooks"
)
var pubsub *gochannel.GoChannel
@ -72,7 +74,10 @@ func InitEvents() (err error) {
return nil
})
whMiddleware := webhooks.GenerateMiddleware()
k3idii marked this conversation as resolved
Review

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

Why not calling `webhooks.GenerateMiddleware()` in the `router.AddMiddleware` below?
router.AddMiddleware(
whMiddleware, //webhooks.WebhookMiddleware,
Review

Please remove this commented line.

Please remove this commented line.
poison,
middleware.Retry{
MaxRetries: 5,
@ -86,6 +91,10 @@ func InitEvents() (err error) {
middleware.Recoverer,
)
router.AddMiddleware(
)
Review

Please remove this - it serves no purpose.

Please remove this - it serves no purpose.
for topic, funcs := range listeners {
for _, handler := range funcs {
router.AddNoPublisherHandler(topic+"."+handler.Name(), topic, pubsub, handler.Handle)

View File

@ -0,0 +1,49 @@
// Vikunja is a to-do list application to facilitate your life.
k3idii marked this conversation as resolved Outdated

Please add the copyright header as in the other files.

Please add the copyright header as in the other files.
// Copyright 2018-2021 Vikunja and contributors. All rights reserved.
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU Affero General Public Licensee as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
k3idii marked this conversation as resolved Outdated

Please remove this commented line.

Please remove this commented line.
// GNU Affero General Public Licensee for more details.
//
// You should have received a copy of the GNU Affero General Public Licensee
// along with this program. If not, see <https://www.gnu.org/licenses/>.
package webhooks
import (
"code.vikunja.io/api/pkg/log"
"github.com/ThreeDotsLabs/watermill/message"
)
func GenerateMiddleware() message.HandlerMiddleware {
runtimeConfig := ProcessConfig()
return func(hf message.HandlerFunc) message.HandlerFunc {
return func(msg *message.Message) ([]*message.Message, error) {
topic := message.SubscribeTopicFromCtx(msg.Context())
handleOutgoingWebhook(runtimeConfig, topic, msg)
newMsg, err := hf(msg)
k3idii marked this conversation as resolved
Review

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.
if err != nil {
return nil, err
}
return newMsg, nil
}
}
Review

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

Please handle the error from that function - at least log it.
}
func handleOutgoingWebhook(cfg []WebhookRuntimeConfig, topic string, msg *message.Message) {
log.Debugf("Webhook handler for '%s'", topic)
for i, entry := range cfg {
log.Debugf("Checking webhook config [%d]", i)
if entry.FilterFunc(topic) {
log.Debugf("Executing webhook [%d]", i)
k3idii marked this conversation as resolved Outdated

Please remove all the commented code here.

Please remove all the commented code here.
entry.ExecuteFunc(topic, msg)
}
}
}

143
pkg/webhooks/runtime.go Normal file
View File

@ -0,0 +1,143 @@
// Vikunja is a to-do list application to facilitate your life.
k3idii marked this conversation as resolved Outdated

Please add the copyright header as in the other files.

Please add the copyright header as in the other files.

Please add the copyright header as in the other files.

Please add the copyright header as in the other files.
// Copyright 2018-2021 Vikunja and contributors. All rights reserved.
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU Affero General Public Licensee as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Affero General Public Licensee for more details.
//
// You should have received a copy of the GNU Affero General Public Licensee
// along with this program. If not, see <https://www.gnu.org/licenses/>.
package webhooks
import (
"bytes"
"fmt"
"net/http"
k3idii marked this conversation as resolved
Review

This should be configurable.

This should be configurable.
Review

Done. in config

Done. in config
"strings"
"time"
Review

What does ct mean?

What does `ct` mean?
Review

content-type header and value

content-type header and value
k3idii marked this conversation as resolved
Review

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

This should be either `x-signature` or `X-Signature`.
"crypto/hmac"
"crypto/sha256"
"encoding/hex"
"code.vikunja.io/api/pkg/config"
"code.vikunja.io/api/pkg/log"
"github.com/ThreeDotsLabs/watermill/message"
)
const (
defaultTimeout = 5 * time.Second
ctHeader = "Content-Type"
ctValue = "application/json"
hmacHeader = "X-Signature"
)
type FilteringFunction func(string) bool
type WebhookCallFunction func(string, *message.Message) error
// Single configuration entry
type SingleConfEntry struct {
k3idii marked this conversation as resolved
Review

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.
Events []string `json:"events"`
k3idii marked this conversation as resolved Outdated

I think you mean interesting? (typo)

I think you mean `interesting`? (typo)
URL string `json:"url"`
Secret string `json:"secret"`
Timeout int `json:"timeout"`

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.

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 :)

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.
}
type WebhookRuntimeConfig struct {
FilterFunc FilteringFunction
ExecuteFunc WebhookCallFunction

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?

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.
}
func getWebhookFilterFunc(cfg SingleConfEntry) FilteringFunction {
return func(topic string) (is_interesting bool) {
for _, filter := range cfg.Events {
log.Debugf("Match pattern:'%s' topic:'%s'", filter, topic)
if filter == "*" {
log.Debugf(" '*' == Always match ")
return true
k3idii marked this conversation as resolved
Review

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.
}
if strings.HasPrefix(topic, filter) {
log.Debugf("Positive match [%s] -> [%s]", filter, topic)
return true
}
}
log.Debugf("No match for [%s]", topic)
return false
Review

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.
Review

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)
Review

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.
}
}
k3idii marked this conversation as resolved
Review

Why err1? Please call this variable just err.

Why `err1`? Please call this variable just `err`.
func getWebhookCallFunc(cfg SingleConfEntry) WebhookCallFunction {
return func(topic string, msg *message.Message) error {
endpointURL := cfg.URL
hmacKey := cfg.Secret
timeout := defaultTimeout
if cfg.Timeout > 0 {
timeout = time.Second * time.Duration(cfg.Timeout)
}
log.Debugf("Webhook Call : %s (key=%s)", endpointURL, hmacKey)
webhookURL := fmt.Sprintf("%s%s", endpointURL, topic)
rawData := msg.Payload
req, err := http.NewRequest(http.MethodPost, webhookURL, bytes.NewBuffer(rawData))
k3idii marked this conversation as resolved Outdated

Why err2? Please call this variable just err.

Why `err2`? Please call this variable just `err`.
if err != nil {
return err
k3idii marked this conversation as resolved Outdated

Please remove the commented line of code.

Please remove the commented line of code.
}
client := &http.Client{
Timeout: timeout,
}

This should include the topic name as well.

This should include the topic name as well.

webhookUrl contains url + topic

webhookUrl contains url + topic
req.Header.Set(ctHeader, ctValue)
if len(hmacKey) > 1 {
signature := GenerateHMAC(rawData, hmacKey)
req.Header.Set(hmacHeader, signature)
}
resp, err = client.Do(req)
defer resp.Body.Close()
if err != nil {
log.Debugf("Webhook failed : %s , +%v", webhookURL, err)
return err
}
log.Debugf("Webhook success : %s ", webhookURL)
return nil
}
}
func GenerateHMAC(data []byte, key string) string {
Review

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
Review

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.
h := hmac.New(sha256.New, []byte(key))
h.Write(data)
return hex.EncodeToString(h.Sum(nil))
}
k3idii marked this conversation as resolved
Review

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.
// Process config and prepare mapping
func ProcessConfig() []WebhookRuntimeConfig {
var items []SingleConfEntry
config.WebhooksConf.GetUnmarshaled(&items)
runtime := make([]WebhookRuntimeConfig, len(items))
log.Debugf("Webhook config items : %+v\n", items)
for i, item := range items {
runtime[i].FilterFunc = getWebhookFilterFunc(item)
runtime[i].ExecuteFunc = getWebhookCallFunc(item)
}
return runtime
}