From e05735276e43fadd39b99c3fba318b67d8dd2445 Mon Sep 17 00:00:00 2001 From: bthari Date: Thu, 10 Oct 2024 11:50:47 +0700 Subject: [PATCH] fix: init webhook validation check (#117) --- api/pkg/webhooks/manager.go | 47 +++++++++++------- api/pkg/webhooks/manager_test.go | 84 +++++++++++++++++++++++++++++--- 2 files changed, 108 insertions(+), 23 deletions(-) diff --git a/api/pkg/webhooks/manager.go b/api/pkg/webhooks/manager.go index bc19ff57..2ce6a321 100644 --- a/api/pkg/webhooks/manager.go +++ b/api/pkg/webhooks/manager.go @@ -132,6 +132,9 @@ func parseAndValidateConfig( if err := validateClients(allClients); err != nil { return nil, err } + if err := validateSyncClients(syncClients); err != nil { + return nil, err + } syncClientMap[eventType] = syncClients asyncClientMap[eventType] = asyncClients } @@ -153,22 +156,39 @@ func validateWebhookResponse(content []byte) error { return fmt.Errorf("webhook response is not a valid json object and not empty") } +// validateClients ensures that there are no duplicate webhook names func validateClients(webhookClients []WebhookClient) error { - // ensure that only 1 sync client has finalResponse set to true + existingWebhooks := make(map[string]bool) + for _, client := range webhookClients { + if existingWebhooks[client.GetName()] { + return fmt.Errorf("duplicate webhook name") + } + existingWebhooks[client.GetName()] = true + } + + return nil +} + +// validateSyncClients ensures that only 1 sync client has finalResponse set to true; +// and for clients that use the response from another client, the used client is +// defined before the client that uses it +func validateSyncClients(webhookClients []WebhookClient) error { + if len(webhookClients) == 0 { + return nil + } + isFinalResponseSet := false - // Check for duplicate webhook names - webhookNames := make(map[string]int) - for idx, client := range webhookClients { + existingWebhooks := make(map[string]bool) + + for _, client := range webhookClients { if client.IsFinalResponse() { if isFinalResponseSet { return fmt.Errorf("only 1 sync client can have finalResponse set to true") } isFinalResponseSet = true } - if _, ok := webhookNames[client.GetName()]; ok { - return fmt.Errorf("duplicate webhook name") - } - webhookNames[client.GetName()] = idx + + existingWebhooks[client.GetName()] = true // Ensure that if a client uses the response from another client, that client exists // If a client uses the response from another client, it must be defined before it if client.GetUseDataFrom() == "" { @@ -177,17 +197,10 @@ func validateClients(webhookClients []WebhookClient) error { // since the payload used will be the user's payload continue } - useIdx, ok := webhookNames[client.GetUseDataFrom()] - if !ok { + + if client.GetName() == client.GetUseDataFrom() || !existingWebhooks[client.GetUseDataFrom()] { return fmt.Errorf("webhook name %s not found", client.GetUseDataFrom()) } - if useIdx > idx { - return fmt.Errorf( - "webhook name %s must be defined before %s", - client.GetUseDataFrom(), - client.GetName(), - ) - } } if !isFinalResponseSet { return fmt.Errorf("at least 1 sync client must have finalResponse set to true") diff --git a/api/pkg/webhooks/manager_test.go b/api/pkg/webhooks/manager_test.go index 9f8960f8..77b08a63 100644 --- a/api/pkg/webhooks/manager_test.go +++ b/api/pkg/webhooks/manager_test.go @@ -130,7 +130,7 @@ func TestInitializeWebhooks(t *testing.T) { expectedError: true, }, { - name: "Config enabled with multiple webhooks", + name: "Config enabled with multiple webhooks all sync", cfg: &Config{ Enabled: true, Config: map[EventType][]WebhookConfig{ @@ -152,6 +152,30 @@ func TestInitializeWebhooks(t *testing.T) { eventList: []EventType{"event1"}, expectedError: false, }, + { + name: "Config enabled with multiple webhooks all async", + cfg: &Config{ + Enabled: true, + Config: map[EventType][]WebhookConfig{ + "event1": { + { + Name: "webhook1", + URL: "http://example.com", + Method: "POST", + Async: true, + }, + { + Name: "webhook2", + URL: "http://example.com", + Method: "POST", + Async: true, + }, + }, + }, + }, + eventList: []EventType{"event1"}, + expectedError: false, + }, { name: "Config enabled with multiple webhooks fail (both set FinalResponse)", cfg: &Config{ @@ -247,6 +271,30 @@ func TestInitializeWebhooks(t *testing.T) { eventList: []EventType{"event1"}, expectedError: true, }, + { + name: "Config enabled with multiple webhooks with sync and async", + cfg: &Config{ + Enabled: true, + Config: map[EventType][]WebhookConfig{ + "event1": { + { + Name: "webhook2", + URL: "http://example.com", + Method: "POST", + Async: true, + }, + { + Name: "webhook1", + URL: "http://example.com", + Method: "POST", + FinalResponse: true, + }, + }, + }, + }, + eventList: []EventType{"event1"}, + expectedError: false, + }, { name: "Config enabled with multiple webhooks with sync and async, fail with duplicate names", cfg: &Config{ @@ -254,11 +302,10 @@ func TestInitializeWebhooks(t *testing.T) { Config: map[EventType][]WebhookConfig{ "event1": { { - Name: "webhook2", - URL: "http://example.com", - Method: "POST", - UseDataFrom: "webhook1", - Async: true, + Name: "webhook2", + URL: "http://example.com", + Method: "POST", + Async: true, }, { Name: "webhook1", @@ -278,6 +325,31 @@ func TestInitializeWebhooks(t *testing.T) { eventList: []EventType{"event1"}, expectedError: true, }, + { + name: "Config enabled with multiple webhooks fail (no final response set for sync webhook)", + cfg: &Config{ + Enabled: true, + Config: map[EventType][]WebhookConfig{ + "event1": { + { + Name: "webhook1", + URL: "http://example.com", + Method: "POST", + FinalResponse: true, + Async: true, + }, + { + Name: "webhook2", + URL: "http://example.com", + Method: "POST", + Async: false, + }, + }, + }, + }, + eventList: []EventType{"event1"}, + expectedError: true, + }, } for _, test := range tests {