Event-based webhook #1183
|
@ -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
|
||||
timeout : 3
|
||||
konrad
commented
Please add comments for the 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
|
|
@ -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`
|
||||
|
||||
|
||||
|
|
|
@ -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
konrad
commented
Since there are multiple webhooks defined under that endpoint, I think this should be 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
konrad
commented
Please remove the commented lines. Please remove the commented lines.
|
||||
// InitConfig initializes the config, sets defaults etc.
|
||||
|
|
|
@ -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
konrad
commented
Why not calling Why not calling `webhooks.GenerateMiddleware()` in the `router.AddMiddleware` below?
|
||||
|
||||
router.AddMiddleware(
|
||||
whMiddleware, //webhooks.WebhookMiddleware,
|
||||
konrad
commented
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(
|
||||
|
||||
)
|
||||
konrad
commented
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)
|
||||
|
|
|
@ -0,0 +1,49 @@
|
|||
// Vikunja is a to-do list application to facilitate your life.
|
||||
k3idii marked this conversation as resolved
Outdated
konrad
commented
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
konrad
commented
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
konrad
commented
I think this function should be called something more descriptive, maybe 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
|
||||
}
|
||||
}
|
||||
konrad
commented
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
konrad
commented
Please remove all the commented code here. Please remove all the commented code here.
|
||||
entry.ExecuteFunc(topic, msg)
|
||||
}
|
||||
}
|
||||
}
|
|
@ -0,0 +1,143 @@
|
|||
// Vikunja is a to-do list application to facilitate your life.
|
||||
k3idii marked this conversation as resolved
Outdated
konrad
commented
Please add the copyright header as in the other files. Please add the copyright header as in the other files.
konrad
commented
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
konrad
commented
This should be configurable. This should be configurable.
k3idii
commented
Done. in config Done. in config
|
||||
"strings"
|
||||
"time"
|
||||
konrad
commented
What does What does `ct` mean?
k3idii
commented
content-type header and value content-type header and value
|
||||
|
||||
k3idii marked this conversation as resolved
konrad
commented
This should be either 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
konrad
commented
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
konrad
commented
I think you mean I think you mean `interesting`? (typo)
|
||||
URL string `json:"url"`
|
||||
Secret string `json:"secret"`
|
||||
Timeout int `json:"timeout"`
|
||||
konrad
commented
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.
k3idii
commented
At first I was thinking this way. But after going over few use-cases I ened it like this way. My justification:
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 :)
konrad
commented
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 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
|
||||
konrad
commented
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?
k3idii
commented
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
konrad
commented
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
|
||||
konrad
commented
Please wrap the payload in a Something like this:
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.
k3idii
commented
My idea behind that was :
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)
konrad
commented
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 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
konrad
commented
Why 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
konrad
commented
Why Why `err2`? Please call this variable just `err`.
|
||||
if err != nil {
|
||||
return err
|
||||
k3idii marked this conversation as resolved
Outdated
konrad
commented
Please remove the commented line of code. Please remove the commented line of code.
|
||||
}
|
||||
|
||||
client := &http.Client{
|
||||
Timeout: timeout,
|
||||
}
|
||||
|
||||
konrad
commented
This should include the topic name as well. This should include the topic name as well.
k3idii
commented
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 {
|
||||
konrad
commented
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
k3idii
commented
Yes, but my low golang skill was not able to reproduce. 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
konrad
commented
Please remove these calls to 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
|
||||
}
|
Please add the secret here as well so that it shows up in the docs.