Event-based webhook #1183
No reviewers
Labels
No Label
dependencies
duplicate
help wanted
invalid
kind/bug
kind/feature
needs reproduction
question
security
wontfix
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/vikunja#1183
Loading…
Reference in New Issue
No description provided.
Delete Branch "k3idii/api:webhook-events"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description
Added webhook support (event based).
Simple HTTP POST of event data to specific endpoint
Checklist
mage do-the-swag
)config.yml.saml
and runningmage generate-docs
)@ -103,6 +104,7 @@ func FullInit() {
go func() {
models.RegisterListeners()
user.RegisterListeners()
webhooks.RegisterListeners()
Using a message router middleware like in this example is probably the better aproach than registering a listener manually for each and every event.
I didn't try that. Mostly cause I'm not (yet) familiar w/ Watermill :)
Sound like better idea (less code)
How can we combine this with #1181? Or would you say this is something different?
#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: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)
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.
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).
Updated.
Using middleware/router way
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/"
Please add the secret here as well so that it shows up in the docs.
@ -161,2 +161,4 @@
MetricsUsername Key = `metrics.username`
MetricsPassword Key = `metrics.password`
WebhookConf Key = `webhooks`
Since there are multiple webhooks defined under that endpoint, I think this should be
webhooks
.@ -372,0 +380,4 @@
WebhookConf.setDefault( []int{} )
// WebhookEnabled.setDefault(false)
// WebhookURL.setDefault("")
Please remove the commented lines.
@ -72,7 +74,10 @@ func InitEvents() (err error) {
return nil
})
whMiddleware := webhooks.GenerateMiddleware()
Why not calling
webhooks.GenerateMiddleware()
in therouter.AddMiddleware
below?@ -75,1 +77,4 @@
whMiddleware := webhooks.GenerateMiddleware()
router.AddMiddleware(
whMiddleware, //webhooks.WebhookMiddleware,
Please remove this commented line.
@ -88,1 +93,4 @@
router.AddMiddleware(
)
Please remove this - it serves no purpose.
@ -0,0 +1,91 @@
package webhooks
Please add the copyright header as in the other files.
@ -0,0 +8,4 @@
"github.com/ThreeDotsLabs/watermill/message"
)
// type middlwareFunc func(message.HandlerFunc) message.HandlerFunc
Please remove this commented line.
@ -0,0 +28,4 @@
func TheGreatWebhookHandler(cfg []WebhookRuntimeConfig, topic string, msg *message.Message) {
I think this function should be called something more descriptive, maybe
HandleOutgoingWebhook
or something like that.@ -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)
Please handle the error from that function - at least log it.
@ -0,0 +42,4 @@
/*
func OldWebhookMiddleware(h message.HandlerFunc) message.HandlerFunc {
Please remove all the commented code here.
@ -0,0 +1,142 @@
Please add the copyright header as in the other files.
Please add the copyright header as in the other files.
@ -0,0 +19,4 @@
)
const (
timeoutVal = 5 * time.Second
This should be configurable.
Done. in config
@ -0,0 +21,4 @@
const (
timeoutVal = 5 * time.Second
ctHeader = "Content-Type"
ctValue = "application/json"
What does
ct
mean?content-type header and value
@ -0,0 +22,4 @@
timeoutVal = 5 * time.Second
ctHeader = "Content-Type"
ctValue = "application/json"
hmacHeader = "X-signature"
This should be either
x-signature
orX-Signature
.@ -0,0 +44,4 @@
func GetWebhookFilterFunc(cfg SingleConfEntry) FilteringFunction {
Does this function need to be public? If not, please make it private.
@ -0,0 +45,4 @@
func GetWebhookFilterFunc(cfg SingleConfEntry) FilteringFunction {
return func(topic string) (iteresting bool) {
I think you mean
interesting
? (typo)@ -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 == "*" {
I think we should just use glob matching for this.
At first I was thinking this way. But after going over few use-cases I ened it like this way. My justification:
category.subcategory.etc
category
orcat.subcat
- not.*user.*
(might be spossible but rather useless).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 ")
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.
@ -0,0 +62,4 @@
}
}
func GetWebhookCallFunc(cfg SingleConfEntry) WebhookCallFunction {
Does this function need to be public? If not, please make it private.
@ -0,0 +70,4 @@
log.Debugf("Webhook Call : %s (%s)", endpointURL, hmacKey)
webhookUrl := fmt.Sprintf("%s%s", endpointURL, topic)
rawData := msg.Payload
Please wrap the payload in a
WebhookEvent
struct or similar with the topic and payload.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.
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)
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)
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.
@ -0,0 +72,4 @@
rawData := msg.Payload
req, err1 := http.NewRequest( http.MethodPost, webhookUrl, bytes.NewBuffer(rawData))
Why
err1
? Please call this variable justerr
.@ -0,0 +89,4 @@
req.Header.Set(hmacHeader, signature)
}
_, err2 := client.Do(req)
Why
err2
? Please call this variable justerr
.@ -0,0 +91,4 @@
_, err2 := client.Do(req)
//_, err := http.Post(webhook_url, "application/json",data) "x-vikunja-signature"
Please remove the commented line of code.
@ -0,0 +97,4 @@
return err2
}
log.Debugf("Webhook success : %s ", webhookUrl )
This should include the topic name as well.
webhookUrl contains url + topic
@ -0,0 +117,4 @@
var items []SingleConfEntry
config.WebhookConf.GetUnmarshaled(&items)
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
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)
Please remove these calls to
fmt
or replace them with calls to appropriate logging functions.@ -322,0 +326,4 @@
# URL for webhook
url: "http://127.0.0.1/test/test1/?q="
secret : "abc123"
timeout : 3
Please add comments for the
secret
andtimeout
properties. Also you set the default value here to3
but said in a comment below the default value is5
? Which one is correct?Also please remove the second example value and regenerate the config docs.
Please fix the build errors (this is the reason CI is failing):
Closing this due to inactivity. Please ping if you want to pick it up again.
Pull request closed